Re: Synchronizing slots from primary to standby

On Tue, Oct 17, 2023 at 9:06 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.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.malik@gmail.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!
>
> +   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().
>
> + * 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?
>

We have added some basic tests in v25. More detailed tests to be added
in coming versions.

> 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.
>
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com