Thread
-
Re: Improve pg_sync_replication_slots() to wait for primary to advance
Japin Li <japinli@hotmail.com> — 2025-10-31T04:42:27Z
On Thu, 30 Oct 2025 at 21:18, Ajin Cherian <itsajin@gmail.com> wrote: > On Mon, Oct 27, 2025 at 8:22 PM Japin Li <japinli@hotmail.com> wrote: >> >> >> Hi, Ajin >> >> Thanks for updating the patch. >> >> On Mon, 27 Oct 2025 at 18:47, Ajin Cherian <itsajin@gmail.com> wrote: >> > On Fri, Oct 24, 2025 at 8:29 PM shveta malik <shveta.malik@gmail.com> wrote: >> >> >> >> On Wed, Oct 22, 2025 at 10:25 AM Ajin Cherian <itsajin@gmail.com> wrote: >> >> > >> >> > >> >> > I've modified the comments to reflect the new changes. >> >> > >> >> > attaching patch v18 with the above changes. >> >> > >> >> >> >> Thanks for the patch. The test is still not clear. Can we please add >> >> the test after the test of "Test logical failover slots corresponding >> >> to different plugins" finishes instead of adding it in between? >> >> >> > >> > I've rewritten the tests again to make this possible. Attaching v19 >> > which has the modified tap test. >> >> Here are some comments on the new patch. >> >> 1. Given the existence of the foreach_ptr macro, we can switch the usage of >> foreach to foreach_ptr. >> >> diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c >> index 1b78ffc5ff1..5db51407a82 100644 >> --- a/src/backend/replication/logical/slotsync.c >> +++ b/src/backend/replication/logical/slotsync.c >> @@ -872,7 +872,6 @@ fetch_remote_slots(WalReceiverConn *wrconn, List *slot_names) >> >> if (slot_names != NIL) >> { >> - ListCell *lc; >> bool first_slot = true; >> >> /* >> @@ -880,10 +879,8 @@ fetch_remote_slots(WalReceiverConn *wrconn, List *slot_names) >> */ >> appendStringInfoString(&query, " AND slot_name IN ("); >> >> - foreach(lc, slot_names) >> + foreach_ptr(char, slot_name, slot_names) >> { >> - char *slot_name = (char *) lfirst(lc); >> - >> if (!first_slot) >> appendStringInfoString(&query, ", "); >> >> @@ -1872,15 +1869,13 @@ static List * >> extract_slot_names(List *remote_slots) >> { >> List *slot_names = NIL; >> - ListCell *lc; >> MemoryContext oldcontext; >> >> /* Switch to long-lived TopMemoryContext to store slot names */ >> oldcontext = MemoryContextSwitchTo(TopMemoryContext); >> >> - foreach(lc, remote_slots) >> + foreach_ptr(RemoteSlot, remote_slot, remote_slots) >> { >> - RemoteSlot *remote_slot = (RemoteSlot *) lfirst(lc); >> char *slot_name; >> >> slot_name = pstrdup(remote_slot->name); >> >> 2. To append a signal character, switch from appendStringInfoString() to the >> more efficient appendStringInfoChar(). >> >> + appendStringInfoString(&query, ")"); >> >> 3. The query memory can be released immediately after walrcv_exec() because >> there are no subsequent references. >> >> @@ -895,6 +892,7 @@ fetch_remote_slots(WalReceiverConn *wrconn, List *slot_names) >> >> /* Execute the query */ >> res = walrcv_exec(wrconn, query.data, SLOTSYNC_COLUMN_COUNT, slotRow); >> + pfree(query.data); >> if (res->status != WALRCV_OK_TUPLES) >> ereport(ERROR, >> errmsg("could not fetch failover logical slots info from the primary server: %s", >> @@ -975,7 +973,6 @@ fetch_remote_slots(WalReceiverConn *wrconn, List *slot_names) >> } >> >> walrcv_clear_result(res); >> - pfree(query.data); >> >> return remote_slot_list; >> } >> > > Thanks for your review, Japin. Here's patch v20 addressing the comments. > Thanks for updating the patch. Here are some comments on v20. 1. Since the content is unchanged, no modification is needed here. - * 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 synchronization in the next cycle. + * 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 + * synchronization in the next cycle. 2. Could we align the parameter comment style for synchronize_slots() and fetch_remote_slots() for better consistency? 3. Is this redundant? It was already initialized to false during declaration. + /* Reset flag before every iteration */ + slot_persistence_pending = false; 4. A minor nitpick. The opening brace should be on a new line for style consistency. + if (!IsTransactionState()) { + StartTransactionCommand(); + started_tx = true; + } 5. Given that fparams.slot_names is a list, I suggest we replace NULL with NIL for type consistency. + fparams.slot_names = NULL; -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.