Re: Synchronizing slots from primary to standby

On Tue, Nov 21, 2023 at 8:32 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> v37 fails to apply to HEAD due to a recent commit e83aa9f92fdd,
> rebased the patches.  PFA v37_2 patches.

Thanks for the patch. Some comments:
subscriptioncmds.c:
CreateSubscription()
and tablesync.c:
process_syncing_tables_for_apply()
                 walrcv_create_slot(wrconn, opts.slot_name, false,
twophase_enabled,
-                                   CRS_NOEXPORT_SNAPSHOT, NULL);
-
-                if (twophase_enabled)
-                    UpdateTwoPhaseState(subid,
LOGICALREP_TWOPHASE_STATE_ENABLED);
-
+                                   failover_enabled,
CRS_NOEXPORT_SNAPSHOT, NULL);

either here or in libpqrcv_create_slot(), shouldn't you check the
remote server version if it supports the failover flag?


+
+            /*
+             * If only the slot_name is specified, it is possible
that the user intends to
+             * use an existing slot on the publisher, so here we
enable failover for the
+             * slot if requested.
+             */
+            else if (opts.slot_name && failover_enabled)
+            {
+                walrcv_alter_slot(wrconn, opts.slot_name, opts.failover);
+                ereport(NOTICE,
+                        (errmsg("enabled failover for replication
slot \"%s\" on publisher",
+                                opts.slot_name)));
+            }

Here, the code only alters the slot if failover = true. You could use
"else if (opts.slot_name && IsSet(opts.specified_opts,
SUBOPT_FAILOVER)" to check if the failover flag is specified and alter
for failover=false as well. Also, shouldn't you check for the server
version if the command ALTER_REPLICATION_SLOT is supported?

slot.c:
ReplicationSlotAlter()

+void
+ReplicationSlotAlter(const char *name, bool failover)
+{
+    Assert(MyReplicationSlot == NULL);
+
+    ReplicationSlotAcquire(name, true);
+
+    if (SlotIsPhysical(MyReplicationSlot))
+        ereport(ERROR,
+                errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                errmsg("cannot use %s with a physical replication slot",
+                       "ALTER_REPLICATION_SLOT"));

shouldn't you release the slot by calling ReplicationSlotRelease
before erroring out?

slot.c:
+/*
+ * A helper function to validate slots specified in standby_slot_names GUCs.
+ */
+static bool
+validate_standby_slots(char **newval)
+{
+    char       *rawname;
+    List       *elemlist;
+    ListCell   *lc;
+
+    /* Need a modifiable copy of string */
+    rawname = pstrdup(*newval);

rawname is not always freed.

launcher.c:

+    SlotSyncWorker->hdr.proc = MyProc;
+
+    before_shmem_exit(slotsync_worker_detach, (Datum) 0);
+
+    LWLockRelease(SlotSyncWorkerLock);
+}

before_shmem_exit() can error out leaving the lock acquired. Maybe you
should release the lock prior to calling before_shmem_exit() because
you don't need the lock there.

regards,
Ajin Cherian
Fujitsu Australia