On Tue, Oct 28, 2025 at 5:25 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Oct 28, 2025 at 3:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Oct 27, 2025 at 8:38 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > On Mon, Oct 27, 2025 at 6:12 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > >
> > > > Another related point is that, say we decide to disable decoding
> > > > because the last logical slot got invalidated and
> > > > RequestDisableLogicalDecoding()->LogicalDecodingStatusChangeAllowed()
> > > > returns false, then how the disabling will happen?
> > > >
> > >
> > > If LogicalDecodingStatusChangeAllowed() returns false, we do not
> > > disable logical decoding because that essentially means
> > > recovery-in-progress and if that is the case, we do not allow
> > > status-change. LogicalDecodingStatusChangeAllowed() returns false
> > > always on standby until it is promoted and recovery ends.
> > >
> >
> > But the request will be lost and we won't be able to disable decoding
> > after promotion.
> >
>
> Okay, I noticed that even if this request is lost the promotion path
> will take care of disabling the decoding via
> UpdateLogicalDecodingStatusEndOfRecovery().
>
> I have noticed a few minor points:
> 1.
> @@ -377,6 +378,12 @@ CheckpointerMain(const void *startup_data, size_t
> startup_data_len)
> */
> AbsorbSyncRequests();
>
> + /*
> + * Disable logical decoding if someone requested it. See comments atop
> + * logicalctl.c.
> + */
> + DisableLogicalDecodingIfNecessary();
> +
>
> The patch calls DisableLogicalDecodingIfNecessary() in the beginning
> of checkpointer loop, so if there is any delay in disabling there is
> more chance that scheduled checkpoint can delay. I think it is better
> to do this before wait like we do for switching wal files via a call
> to CheckArchiveTimeout().
Agreed.
>
> 2.
> @@ -7054,6 +7097,7 @@ CreateCheckPoint(int flags)
>
> checkPoint.fullPageWrites = Insert->fullPageWrites;
> checkPoint.wal_level = wal_level;
> + checkPoint.logicalDecodingEnabled = IsLogicalDecodingEnabled();
>
> This is the place where we have blocked all concurrent WAL writes via
> WALInsertLockAcquireExclusive(), so why to set it here when it can be
> done after releasing the lock with other parameters in checkpoint
> record aka after 'Get the other info we need for the checkpoint
> record.'. I guess you have done to set logicalDecodingEnabled along
> with wal_level but I think it is better to set it after releasing the
> lock along with other parameters unless there is a reason to do it
> under WALInsertLockAcquireExclusive.
Agreed.
I'm updating the patch, and will share the updated patch soon.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
