On Thu, Dec 4, 2025 at 1:30 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Thu, Dec 4, 2025 at 1:54 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> >
> > I've attached the updated patch that incorporated the review comments
> > and is rebased to the current HEAD.
> >
>
> Thanks, a few things:
>
>
> 1)
> + The system automatically increases the
> + effective WAL level to <literal>logical</literal> when
> creating the first
> + logical replication slot, and decreases it back to
> <literal>replica</literal>
> + when dropping the last logical replication slot. The current
> effective WAL
> + level can be monitored through <xref
> linkend="guc-effective-wal-level"/>
> + parameter.
>
> Shouldn't we mention about invalidation of slot along with dropping of
> slot? I have suggested this earlier but I think it got missed to be
> addressed.
Fixed. Sorry I missed it.
>
> 2)
> + else if (sync_replication_slots)
> + {
> + /*
> + * Signal the postmaster to launch the slotsync worker.
> + *
> + * XXX: For simplicity, we keep the slotsync worker running
> + * even after logical decoding is disabled. A future
> + * improvement can consider starting and stopping the worker
> + * based on logical decoding status change.
> + */
> + kill(PostmasterPid, SIGUSR1);
> + }
> + }
> +
> + /* Update the status on shared memory */
> + UpdateLogicalDecodingStatus(logical_decoding, true);
>
> I see that we have moved 'Update' post slotsync's start attempt. This
> leaves a possibility that that slot-sync is started sooner than last
> 'Update' call and thus slotsync may exit with:
> replication slot synchronization requires "effective_wal_level" >=
> "logical" on the primary
>
> I see that update was prior to the slotsync step in earlier patches.
> Why have we moved it to a later stage?
You're right. I've reconsidered the operation order and reverted the
previous change. On further thought, I think it should not happen to
decode the STATUS_CHANGE record because when applying the
STATUS_CHANGE record, we disable logical decoding first and then
invalidate the slots. There is no window where a logical slot can
creep in on the standby, and on the primary it cannot restart after
changing wal_level < replica if there is a pre-existing slot. If my
understanding is right, we should put a sanity check in xlog_decode().
>
> 3)
> + /* Return if someone already started to enable logical decoding */
>
> Shall we update:
> Return if someone already started to enable logical decoding, or if it
> is already enabled.
Fixed.
I've attached the updated patches. The 0002 patch is a patch to
address the above review comments. If it looks good, I'll merge these
changes.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
