MIME-Version: 1.0
References: <CAJpy0uBefXUS_TSz=oxmYKHdg-fhxUT0qfjASW3nmqnzVC3p6A@mail.gmail.com>
 <CAA4eK1+MVF7xfeB+oprEHfdCL4tdiZv1kQ8=2FhdTq=XL18u2w@mail.gmail.com>
 <CAJpy0uBGz_aZM99D+djJbJvhddoyVRqdzoPwTsT10J5Tt62faw@mail.gmail.com>
 <CALj2ACW4Tpdh4nsn7RaTB8C2NufVnNZtGYqPNwcbdaH4Ce=Yqw@mail.gmail.com> <CAJpy0uD-bRDc+rdf5znj7gp40nTq=m-1QE4U6z029tYV+C4xPg@mail.gmail.com>
In-Reply-To: <CAJpy0uD-bRDc+rdf5znj7gp40nTq=m-1QE4U6z029tYV+C4xPg@mail.gmail.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Fri, 12 Apr 2024 16:17:53 +0530
Message-ID: <CAA4eK1LbZ4bgDFtG5HM=ye6mrHR4WTiAv1V7dY4PPod4F++5dA@mail.gmail.com>
Subject: Re: promotion related handling in pg_sync_replication_slots()
To: shveta malik <shveta.malik@gmail.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, 
	pgsql-hackers <pgsql-hackers@postgresql.org>, 
	"Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAA4eK1LbZ4bgDFtG5HM%3Dye6mrHR4WTiAv1V7dY4PPod4F%2B%2B5dA%40mail.gmail.com>
Precedence: bulk

On Fri, Apr 12, 2024 at 7:57=E2=80=AFAM shveta malik <shveta.malik@gmail.co=
m> wrote:
>
> On Sat, Apr 6, 2024 at 12:25=E2=80=AFPM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Fri, Apr 5, 2024 at 10:31=E2=80=AFAM shveta malik <shveta.malik@gmai=
l.com> wrote:
> > >
> > > Please find v2. Changes are:
> >
> > Thanks for the patch. Here are some comments.
>
> Thanks for reviewing.
> >
> > 1. Can we have a clear saying in the shmem variable who's syncing at
> > the moment? Is it a slot sync worker or a backend via SQL function?
> > Perhaps turn "bool syncing;" to "SlotSyncSource sync_source;"
> >
> > typedef enum SlotSyncSource
> > {
> >     SLOT_SYNC_NONE,
> >     SLOT_SYNC_WORKER,
> >     SLOT_SYNC_BACKEND,
> > } SlotSyncSource;
> >
> > Then, the check in ShutDownSlotSync can be:
> >
> > +       /*
> > +        * Return if neither the slot sync worker is running nor the fu=
nction
> > +        * pg_sync_replication_slots() is executing.
> > +        */
> > +       if ((SlotSyncCtx->pid =3D=3D InvalidPid) &&
> > SlotSyncCtx->sync_source !=3D SLOT_SYNC_BACKEND)
> >         {
> >

I don't know if this will be help, especially after fixing the race
condition I mentioned. But otherwise, also, at this stage it doesn't
seem helpful to add the source of sync explicitly.

> > 2.
> > SyncReplicationSlots(WalReceiverConn *wrconn)
> >  {
> > +    /*
> > +     * Startup process signaled the slot sync to stop, so if meanwhile=
 user
> > +     * has invoked slot sync SQL function, simply return.
> > +     */
> > +    SpinLockAcquire(&SlotSyncCtx->mutex);
> > +    if (SlotSyncCtx->stopSignaled)
> > +    {
> > +        ereport(LOG,
> > +                errmsg("skipping slot synchronization as slot sync
> > shutdown is signaled during promotion"));
> > +
> >
> > Unless I'm missing something, I think this can't detect if the backend
> > via SQL function is already half-way through syncing in
> > synchronize_one_slot. So, better move this check to (or also have it
> > there) slot sync loop that calls synchronize_one_slot. To avoid
> > spinlock acquisitions, we can perhaps do this check in when we acquire
> > the spinlock for synced flag.
>
> If the sync via SQL function is already half-way, then promotion
> should wait for it to finish. I don't think it is a good idea to move
> the check to synchronize_one_slot().  The sync-call should either not
> start (if it noticed the promotion) or finish the sync and then let
> promotion proceed. But I would like to know others' opinion on this.
>

Agreed.

--=20
With Regards,
Amit Kapila.