Re: Synchronizing slots from primary to standby

On Mon, Feb 5, 2024 at 7:59 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>

I have pushed the first patch. Next, a few comments on 0002 are as follows:
1.
+static bool
+validate_parameters_and_get_dbname(char **dbname, int elevel)

For 0002, we don't need dbname as out parameter. Also, we can rename
the function to validate_slotsync_params() or something like that.
Also, for 0003, we don't need to get the dbname from
wait_for_valid_params_and_get_dbname(), instead there could be a
common function that can be invoked from validate_slotsync_params()
and caller of wait function that caches the value of dbname.

The other parameter elevel is also not required for 0002.

2.
+ /*
+ * Make sure that concerned WAL is received and flushed before syncing
+ * slot to target lsn received from the primary server.
+ */
+ latestFlushPtr = GetStandbyFlushRecPtr(NULL);
+ if (remote_slot->confirmed_lsn > latestFlushPtr)
+ {
+ /*
+ * Can get here only if GUC 'standby_slot_names' on the primary server
+ * was not configured correctly.
+ */
+ ereport(LOG,
+ errmsg("skipping slot synchronization as the received slot sync"
+    " LSN %X/%X for slot \"%s\" is ahead of the standby position %X/%X",
+    LSN_FORMAT_ARGS(remote_slot->confirmed_lsn),
+    remote_slot->name,
+    LSN_FORMAT_ARGS(latestFlushPtr)));
+
+ return false;

In the case of a function invocation, this should be an ERROR. We can
move the comment related to 'standby_slot_names' to a later patch
where that GUC is introduced. See, if there are other LOGs in the
patch that needs to be converted to ERROR.

3. The function pg_sync_replication_slots() should be in file
slotfuncs.c and common functionality between this function and
slotsync worker can be exposed via a function in slotsync.c.

4.
/*
+ * Using the specified primary server connection, check whether we are
+ * cascading standby and validates primary_slot_name for
+ * non-cascading-standbys.
+ */
+ check_primary_info(wrconn, &am_cascading_standby,
+    &primary_slot_invalid, ERROR);
+
+ if (am_cascading_standby)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot synchronize replication slots to a cascading standby"));

primary_slot_invalid is not used in this patch. I think we can allow
the function can be executed on cascading_standby as well because this
will be used for the planned switchover.

5. I don't see any problem with allowing concurrent processes trying
to sync the same slot at the same time as each process will acquire
the slot and only one process can acquire the slot at a time, the
other will get an ERROR.

--
With Regards,
Amit Kapila.