Re: Synchronizing slots from primary to standby

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!

+ 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?

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