On Mon, Sep 22, 2025 at 4:21 PM Ajin Cherian <[email protected]> wrote:
>
>
> Created a patch v13 with these changes.
>
Please find a few comments:
1)
+ /* update the failure structure so that it can be freed on error */
+ fparams.slot_names = slot_names;
+
Since slot_names is assigned only once, we can make the above
assignment as well only once, inside the if-block where we initialize
slot_names.
2)
extract_slot_names():
+ foreach(lc, remote_slots)
+ {
+ RemoteSlot *remote_slot = (RemoteSlot *) lfirst(lc);
+ char *slot_name;
+
+ /* Switch to long-lived TopMemoryContext to store slot names */
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
+ slot_name = pstrdup(remote_slot->name);
+ slot_names = lappend(slot_names, slot_name);
+
+ MemoryContextSwitchTo(oldcontext);
+ }
It will be better to move 'MemoryContextSwitchTo' calls outside of the
loop. No need to switch the context for each slot.
3)
ProcessSlotSyncAPIChanges() gives a feeling that it is actually
processing API changes where instead it is processing interrupts or
config changes. Can we please rename to ProcessSlotSyncAPIInterrupts()
4)
I prefer version 11's slotsync_api_reread_config() over current
slotsync_api_config_changed(). There, even error was taken care of
inside the function, which to me looked better and similar to how
slotsync worker deals with it.
I have made some comment changes, attached the patch. Please include
it if you find it okay.
thanks
Shveta
From 4f5634c33092e536b8662244dd7436f045195690 Mon Sep 17 00:00:00 2001
From: Shveta Malik <[email protected]>
Date: Tue, 23 Sep 2025 10:24:19 +0530
Subject: [PATCH] comments changes.
---
src/backend/replication/logical/slotsync.c | 28 +++++++++++-----------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/backend/replication/logical/slotsync.c
b/src/backend/replication/logical/slotsync.c
index 19792a95635..f980c0f73c4 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -42,8 +42,8 @@
* If the pg_sync_replication API is used to sync the slots, and if the slots
* are not ready to be synced and are marked as RS_TEMPORARY because of any of
* the reasons mentioned above, then the API also waits and retries until the
- * slots are ready to be synced. Refer to the comments in
SyncReplicationSlots()
- * for more details.
+ * slots are marked as RS_PERSISTENT (which means sync-ready). Refer to the
+ * comments in SyncReplicationSlots() for more details.
*
* Any standby synchronized slots will be dropped if they no longer need
* to be synchronized. See comment atop drop_local_obsolete_slots() for more
@@ -572,7 +572,7 @@ reserve_wal_for_local_slot(XLogRecPtr restart_lsn)
* local ones, then update the LSNs and persist the local synced slot for
* future synchronization; otherwise, do nothing.
*
- * slot_persistence_pending is set to true if any of the slots fail to
+ * *slot_persistence_pending is set to true if any of the slots fail to
* persist. It is utilized by the pg_sync_replication_slots() API.
*
* Return true if the slot is marked as RS_PERSISTENT (sync-ready), otherwise
@@ -604,7 +604,9 @@ update_and_persist_local_synced_slot(RemoteSlot
*remote_slot, Oid remote_dbid,
* the next cycle. It may take more time to create such a
* slot. Therefore, we keep this slot and attempt the
* synchronization in the next cycle. Update the
- * slot_persistence_pending flag, so the API can retry.
+ *
+ * We also update the slot_persistence_pending parameter, so
+ * the API can retry.
*/
if (slot_persistence_pending)
*slot_persistence_pending = true;
@@ -623,7 +625,7 @@ update_and_persist_local_synced_slot(RemoteSlot
*remote_slot, Oid remote_dbid,
errdetail("Synchronization could lead to data
loss, because the standby could not build a consistent snapshot to decode WALs
at LSN %X/%08X.",
LSN_FORMAT_ARGS(slot->data.restart_lsn)));
- /* set this, so that API can retry */
+ /* Set this, so that API can retry */
if (slot_persistence_pending)
*slot_persistence_pending = true;
@@ -650,7 +652,7 @@ update_and_persist_local_synced_slot(RemoteSlot
*remote_slot, Oid remote_dbid,
* updated. The slot is then persisted and is considered as sync-ready for
* periodic syncs.
*
- * slot_persistence_pending is set to true if any of the slots fail to
+ * *slot_persistence_pending is set to true if any of the slots fail to
* persist. It is utilized by the pg_sync_replication_slots() API.
*
* Returns TRUE if the local slot is updated.
@@ -1953,17 +1955,17 @@ SyncReplicationSlots(WalReceiverConn *wrconn)
validate_remote_info(wrconn);
- /* Retry until all slots are sync ready at least */
+ /* Retry until all the slots are sync-ready */
for (;;)
{
int rc;
bool started_tx = false;
bool slot_persistence_pending = false;
- /* reset flag before every iteration */
+ /* Reset flag before every iteration */
slot_persistence_pending = false;
- /* check for interrupts and config changes */
+ /* Check for interrupts and config changes */
ProcessSlotSyncAPIChanges();
/*
@@ -1994,10 +1996,9 @@ SyncReplicationSlots(WalReceiverConn *wrconn)
* for future iterations (only needed if we haven't
done it yet)
*/
if (slot_names == NIL && slot_persistence_pending)
- /* Extract slot names from the remote slots */
slot_names = extract_slot_names(remote_slots);
- /* update the failure structure so that it can be freed
on error */
+ /* Update the failure structure so that it can be freed
on error */
fparams.slot_names = slot_names;
/* Free the current remote_slots list */
@@ -2007,11 +2008,11 @@ SyncReplicationSlots(WalReceiverConn *wrconn)
if (started_tx)
CommitTransactionCommand();
- /* Done if all slots are at least sync ready */
+ /* Done if all slots are persisted i.e. are sync-ready
*/
if (!slot_persistence_pending)
break;
- /* wait before retrying */
+ /* Wait before retrying */
rc = WaitLatch(MyLatch,
WL_LATCH_SET | WL_TIMEOUT |
WL_EXIT_ON_PM_DEATH,
SLOTSYNC_API_NAPTIME_MS,
@@ -2022,7 +2023,6 @@ SyncReplicationSlots(WalReceiverConn *wrconn)
}
- /* Clean up slot_names if allocated in TopMemoryContext */
if (slot_names)
list_free_deep(slot_names);
--
2.34.1
