Re: Synchronizing slots from primary to standby

On Tue, Oct 17, 2023 at 9:06 PM Drouvot, Bertrand
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi,
>
> On 10/13/23 10:35 AM, shveta malik wrote:
> > On Thu, Oct 12, 2023 at 9:18 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >>
> >
> > PFA v24 patch set which has below changes:
> >
> > 1) 'enable_failover' displayed in pg_replication_slots.
> > 2) Support for 'enable_failover' in
> > pg_create_logical_replication_slot(). It is an optional argument with
> > default value false.
> > 3) Addressed pending comments (1-30) from Peter in [1].
> > 4) Fixed an issue in patch002 due to which even slots with
> > enable_failover=false were getting synced.
> >
> > The changes for 1 and 2 are in patch001 while 3 and 4 are in patch0002
> >
> > Thanks Ajin, for working on 1 and 3.
>
> Thanks for the hard work!
>

Thanks for the feedback. I will try to address these in the next 1-2 versions.

> + if (RecoveryInProgress())
> + wrconn = slotsync_remote_connect(NULL);
>
> does produce at compilation time:
>
> launcher.c:1916:40: warning: too many arguments in call to 'slotsync_remote_connect'
> wrconn = slotsync_remote_connect(NULL);
>
> Looking at 0001:
>
> commit message:
>
> "is added at the slot level which
> will be persistent information"
>
> what about "which is persistent information" ?
>
> Code:
>
> + True if this logical slot is enabled to be synced to the physical standbys
> + so that logical replication is not blocked after failover. Always false
> + for physical slots.
>
> Not sure "not blocked" is the right wording. "can be resumed from the new primary" maybe?
>
> +static void
> +ProcessRepliesAndTimeOut(void)
> +{
> + CHECK_FOR_INTERRUPTS();
> +
> + /* Process any requests or signals received recently */
> + if (ConfigReloadPending)
> + {
> + ConfigReloadPending = false;
> + ProcessConfigFile(PGC_SIGHUP);
> + SyncRepInitConfig();
> + SlotSyncInitConfig();
> + }
>
> Do we want to do this at each place ProcessRepliesAndTimeOut() is being
> called? I mean before this change it was not done in ProcessPendingWrites().
>

Are you referring to ConfigReload stuff ? I see that even in
ProcessPendingWrites(), we do it after WalSndWait(). Now only the
order is changed, it is before WalSndWait() now.

> + * Wait for physical standby to confirm receiving give lsn.
>
> typo? s/give/given/
>
>
> diff --git a/src/test/recovery/t/050_verify_slot_order.pl b/src/test/recovery/t/050_verify_slot_order.pl
> new file mode 100644
> index 0000000000..25b3d5aac2
> --- /dev/null
> +++ b/src/test/recovery/t/050_verify_slot_order.pl
> @@ -0,0 +1,145 @@
> +
> +# Copyright (c) 2023, PostgreSQL Global Development Group
> +
>
> Regarding the TAP tests, should we also add some testing related to enable_failover being set
> in pg_create_logical_replication_slot() and pg_logical_slot_get_changes() behavior too?
>

Sure, will do it.

> Please note that current comments are coming while
> "quickly" going through 0001.
>
> I'm planning to have a closer look at 0001 and 0002 too.
>

Yes, that will be really helpful. Thanks.

thanks
Shveta