On Fri, Feb 9, 2024 at 10:00 AM Zhijie Hou (Fujitsu)
<houzj.f...@fujitsu.com> wrote:
>
Few comments on 0001
===================
1. Shouldn't pg_sync_replication_slots() check whether the user has
replication privilege?
2. The function declarations in slotsync.h don't seem to be in the
same order as they are defined in slotsync.c. For example, see
ValidateSlotSyncParams(). The same is true for new functions exposed
via walreceiver.h and walsender.h. Please check the patch for other
such inconsistencies.
3.
+# Wait for the standby to finish sync
+$standby1->wait_for_log(
+ qr/LOG: ( [A-Z0-9]+:)? newly created slot \"lsub1_slot\" is sync-ready now/,
+ $offset);
+
+$standby1->wait_for_log(
+ qr/LOG: ( [A-Z0-9]+:)? newly created slot \"lsub2_slot\" is sync-ready now/,
+ $offset);
+
+# Confirm that the logical failover slots are created on the standby and are
+# flagged as 'synced'
+is($standby1->safe_psql('postgres',
+ q{SELECT count(*) = 2 FROM pg_replication_slots WHERE slot_name IN
('lsub1_slot', 'lsub2_slot') AND synced;}),
+ "t",
+ 'logical slots have synced as true on standby');
Isn't the last test that queried pg_replication_slots sufficient? I
think wait_for_log() would be required for slotsync worker or am I
missing something?
Apart from the above, I have modified a few comments in the attached.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/replication/logical/slotsync.c
b/src/backend/replication/logical/slotsync.c
index b8b5bd61b4..0d5ca29c18 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -13,14 +13,15 @@
* the slots on the standby and synchronize them. This is done by a call to SQL
* function pg_sync_replication_slots.
*
- * If on the physical standby, the restart_lsn and/or local catalog_xmin is
- * ahead of those on the remote then we cannot create the local standby slot
- * in sync with the primary server because that would mean moving the local
- * slot backwards and the standby might not have WALs retained for old LSN.
- * In this case, the slot will be marked as RS_TEMPORARY. Once the primary
- * server catches up, the slot will be marked as RS_PERSISTENT (which
- * means sync-ready) after which we can call pg_sync_replication_slots()
- * periodically to perform syncs.
+ * If on physical standby, the WAL corresponding to the remote's restart_lsn
+ * is not available or the remote's catalog_xmin precedes the oldest xid for
which
+ * it is guaranteed that rows wouldn't have been removed then we cannot create
+ * the local standby slot because that would mean moving the local slot
+ * backward and decoding won't be possible via such a slot. In this case, the
+ * slot will be marked as RS_TEMPORARY. Once the primary server catches up,
+ * the slot will be marked as RS_PERSISTENT (which means sync-ready) after
+ * which we can call pg_sync_replication_slots() periodically to perform
+ * syncs.
*
* Any standby synchronized slots will be dropped if they no longer need
* to be synchronized. See comment atop drop_local_obsolete_slots() for more
@@ -60,15 +61,10 @@
#include "utils/timeout.h"
#include "utils/varlena.h"
-/*
- * Struct for sharing information to control slot synchronization.
- *
- * The 'syncing' flag should be set to true in any process that is syncing
- * slots to prevent concurrent slot sync, which could lead to errors and slot
- * info overwrite.
- */
+/* Struct for sharing information to control slot synchronization. */
typedef struct SlotSyncCtxStruct
{
+ /* prevents concurrent slot syncs to avoid slot overwrites */
bool syncing;
slock_t mutex;
} SlotSyncCtxStruct;
@@ -933,8 +929,8 @@ SlotSyncInitialize(void)
}
/*
- * Synchronize the replication slots with failover enabled using the
- * specified primary server connection.
+ * Synchronize the failover enabled replication slots using the specified
+ * primary server connection.
*/
void
SyncReplicationSlots(WalReceiverConn *wrconn)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index c443e949b2..94447f6228 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -266,10 +266,10 @@ ReplicationSlotCreate(const char *name, bool db_specific,
ReplicationSlotValidateName(name, ERROR);
/*
- * Do not allow users to create the slots with failover enabled on the
- * standby as we do not support sync to the cascading standby.
+ * Do not allow users to create the failover enabled slots on the
standby
+ * as we do not support sync to the cascading standby.
*
- * However, slots with failover enabled can be created during slot
+ * However, failover enabled slots can be created during slot
* synchronization because we need to retain the same values as the
remote
* slot.
*/
@@ -736,8 +736,8 @@ ReplicationSlotAlter(const char *name, bool failover)
errdetail("This slot is being synced
from the primary server."));
/*
- * Do not allow users to alter slots to enable failover on the
standby
- * as we do not support sync to the cascading standby.
+ * Do not allow users to enable failover on the standby as we
do not
+ * support sync to the cascading standby.
*/
if (failover)
ereport(ERROR,
diff --git a/src/backend/replication/slotfuncs.c
b/src/backend/replication/slotfuncs.c
index cf528db17a..323ce9c1fc 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -794,13 +794,13 @@ copy_replication_slot(FunctionCallInfo fcinfo, bool
logical_slot)
* hence pass find_startpoint false. confirmed_flush will be
set
* below, by copying from the source slot.
*
- * To avoid potential issues with the slot synchronization when
the
- * restart_lsn of a replication slot goes backwards, we set the
- * failover option to false here. This situation occurs when a
slot on
+ * To avoid potential issues with the slot synchronization
where the
+ * restart_lsn of a replication slot can go backward, we set the
+ * failover option to false here. This situation occurs when a
slot on
* the primary server is dropped and immediately replaced with
a new
* slot of the same name, created by copying from another
existing
- * slot. However, the slot synchronization will only observe the
- * restart_lsn of the same slot going backwards.
+ * slot. However, the slot synchronization will only observe
the
+ * restart_lsn of the same slot going backward.
*/
create_logical_replication_slot(NameStr(*dst_name),
plugin,
@@ -955,8 +955,8 @@ pg_copy_physical_replication_slot_b(PG_FUNCTION_ARGS)
}
/*
- * Synchronize replication slots with failover enabled to a standby server from
- * the primary server.
+ * Synchronize failover enabled replication slots with to a standby server
+ * from the primary server.
*/
Datum
pg_sync_replication_slots(PG_FUNCTION_ARGS)
