Re: Allow logical failover slots to wait on synchronous replication

On Wed, Sep 11, 2024 at 2:40 AM John H <johnhyvr(at)gmail(dot)com> wrote:
>
> Hi Shveta,
>
> On Sun, Sep 8, 2024 at 11:16 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> >
> > I was trying to have a look at the patch again, it doesn't apply on
> > the head, needs rebase.
> >
>
> Rebased with the latest changes.
>
> > Regarding 'mode = SyncRepWaitMode', FWIW, SyncRepWaitForLSN() also
> > does in a similar way. It gets mode in local var initially and uses it
> > later. See [1]. So isn't there a chance too that 'SyncRepWaitMode' can
> > change in between.
> >
> > [1]:
> > mode = SyncRepWaitMode;
> > .....
> > ....
> > if (!WalSndCtl->sync_standbys_defined ||
> > lsn <= WalSndCtl->lsn[mode])
> > {
> > LWLockRelease(SyncRepLock);
> > return;
> > }
>
> You are right, thanks for the correction. I tried reproducing with GDB
> where SyncRepWaitMode
> changes due to pg_ctl reload but was unable to do so. It seems like
> SIGHUP only sets ConfigReloadPending = true,
> which gets processed in the next loop in WalSndLoop() and that's
> probably where I was getting confused.

yes, SIGHUP will be processed in the caller of
StandbySlotsHaveCaughtup() (see ProcessConfigFile() in
WaitForStandbyConfirmation()). So we can use 'SyncRepWaitMode'
directly as it is not going to change in StandbySlotsHaveCaughtup()
even if user triggers the change. And thus it was okay to use it even
in the local variable too similar to how SyncRepWaitForLSN() does it.

> In the latest patch, I've added:
>
> Assert(SyncRepWaitMode >= 0);
>
> which should be true since we call SyncRepConfigured() at the
> beginning of StandbySlotsHaveCaughtup(),
> and used SyncRepWaitMode directly.

Yes, it should be okay I think. As SyncRepRequested() in the beginning
makes sure synchronous_commit > SYNCHRONOUS_COMMIT_LOCAL_FLUSH and
thus SyncRepWaitMode should be mapped to either of
WAIT_WRITE/FLUSH/APPLY etc. Will review further.

thanks
Shveta