On Sun, Feb 11, 2024 at 6:53 PM Zhijie Hou (Fujitsu)
<houzj.f...@fujitsu.com> wrote:
>
> Agreed. Here is the V84 patch which addressed this.
>
Few comments:
=============
1. Isn't the new function (pg_sync_replication_slots()) allowed to
sync the slots from physical standby to another cascading standby?
Won't it be better to simply disallow syncing slots on cascading
standby to keep it consistent with slotsync worker behavior?
2.
Previously, I commented to keep the declaration and definition of
functions in the same order but I see that it still doesn't match in
the below case:
@@ -44,6 +46,7 @@ extern void WalSndWakeup(bool physical, bool logical);
extern void WalSndInitStopping(void);
extern void WalSndWaitStopping(void);
extern void HandleWalSndInitStopping(void);
+extern XLogRecPtr GetStandbyFlushRecPtr(TimeLineID *tli);
extern void WalSndRqstFileReload(void);
I think we can keep the new declaration just before WalSndSignals().
That would be more consistent.
3.
+ <para>
+ True if this is a logical slot that was synced from a primary server.
+ </para>
+ <para>
+ On a hot standby, the slots with the synced column marked as true can
+ neither be used for logical decoding nor dropped by the user. The value
I don't think we need a separate para here.
Apart from this, I have made several cosmetic changes in the attached.
Please include these in the next version unless you see any problems.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/replication/logical/slotsync.c
b/src/backend/replication/logical/slotsync.c
index b925c7fe7d..06103d6837 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -80,8 +80,8 @@ typedef struct RemoteSlot
} RemoteSlot;
/*
- * If necessary, update local synced slot metadata based on the data from the
- * remote slot.
+ * If necessary, update the local synced slot's metadata based on the data
+ * from the remote slot.
*
* If no update was needed (the data of the remote slot is the same as the
* local slot) return false, otherwise true.
@@ -99,7 +99,8 @@ update_local_synced_slot(RemoteSlot *remote_slot, Oid
remote_dbid)
xmin_changed = (remote_slot->catalog_xmin != slot->data.catalog_xmin);
restart_lsn_changed = (remote_slot->restart_lsn !=
slot->data.restart_lsn);
- if (!xmin_changed && !restart_lsn_changed &&
+ if (!xmin_changed &&
+ !restart_lsn_changed &&
remote_dbid == slot->data.database &&
remote_slot->two_phase == slot->data.two_phase &&
remote_slot->failover == slot->data.failover &&
@@ -131,7 +132,7 @@ update_local_synced_slot(RemoteSlot *remote_slot, Oid
remote_dbid)
}
/*
- * Get the list of local logical slots which are synchronized from the
+ * Get the list of local logical slots that are synchronized from the
* primary server.
*/
static List *
@@ -302,7 +303,7 @@ reserve_wal_for_local_slot(XLogRecPtr restart_lsn)
* Normally, we can determine it by using the last removed
segment
* number. However, if no WAL segment files have been removed
by a
* checkpoint since startup, we need to search for the oldest
segment
- * file currently existing in XLOGDIR.
+ * file from the current timeline existing in XLOGDIR.
*/
oldest_segno = XLogGetLastRemovedSegno() + 1;
@@ -353,7 +354,7 @@ update_and_persist_local_synced_slot(RemoteSlot
*remote_slot, Oid remote_dbid)
* We do not drop the slot because the restart_lsn can be ahead
of the
* current location when recreating the slot in the next cycle.
It may
* take more time to create such a slot. Therefore, we keep
this slot
- * and attempt the wait and synchronization in the next cycle.
+ * and attempt the synchronization in the next cycle.
*/
return;
}
@@ -736,7 +737,7 @@ validate_primary_slot_name(WalReceiverConn *wrconn)
/*
* Check all necessary GUCs for slot synchronization are set
- * appropriately, otherwise raise ERROR.
+ * appropriately, otherwise, raise ERROR.
*/
void
ValidateSlotSyncParams(void)
@@ -768,10 +769,7 @@ ValidateSlotSyncParams(void)
errmsg("bad configuration for slot
synchronization"),
errhint("\"%s\" must be enabled.",
"hot_standby_feedback"));
- /*
- * Logical decoding requires wal_level >= logical and we currently only
- * synchronize logical slots.
- */
+ /* Logical slot sync/creation requires wal_level >= logical. */
if (wal_level < WAL_LEVEL_LOGICAL)
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 04738ab764..7df22a0251 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -738,7 +738,7 @@ ReplicationSlotAlter(const char *name, bool failover)
/*
* Do not allow users to enable failover for temporary slots as we do
not
- * support sync temporary slots to the standby.
+ * support syncing temporary slots to the standby.
*/
if (failover && MyReplicationSlot->data.persistency == RS_TEMPORARY)
ereport(ERROR,
