Re: Improve pg_sync_replication_slots() to wait for primary to advance - Mailing list pgsql-hackers

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.

regards,
Ajin Cherian
Fujitsu Australia