Hi,
On 10/13/23 10:35 AM, shveta malik wrote:
On Thu, Oct 12, 2023 at 9:18 AM shveta malik <shveta.ma...@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?
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
