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

On Tue, Dec 9, 2025 at 4:04 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> Hi all,
>
> Since commit 04396ea [1] has been pushed, which included part of the
> changes this patch set was addressing, I have updated and rebased the
> patch set to incorporate those changes.
>
> The patch set now contains two patches:
>
> 0001 – Modify the pg_sync_replication_slots API to also handle
> promotion signals and stop synchronization, similar to the slot sync
> worker.
> 0002 – Improve pg_sync_replication_slots to wait for and persist slots
> until they are sync-ready.
>
> Please review the updated patch set (v31).
>

Thanks. Please find a few comments on 001:

1)
Commit message:
"That meant backends
that perform slot synchronization via the pg_sync_replication_slots()
SQL function were not signalled at all because their PIDs were not
recorded in the slot-sync context."

We should phrase it in the singular ('backend'), since multiple
backends cannot run simultaneously because concurrent sync is not
allowed.
Using the term 'their PIDs' gives an impression that there could be
multiple PIDs, which is not the case.

2)
  primary_slotname_changed = strcmp(old_primary_slotname, PrimarySlotName) != 0;
+
  pfree(old_primary_conninfo);

This change to put blank line is not needed.

3)

+ /* Check for sync_replication_slots change */
+ /* Check for parameter changes common to both API and worker */

IMO, these comments are not needed as it is self-explanatory. Even if
we plan to put these, both should be same, either both mentioning API
and worker or neither.

4)
- * receives a stop request from the startup process, or when there is an
+ * because receives a stop request from the startup process, or when
there is an

I think, this change is done by mistake.

5)
+ * Signal slotsync worker or backend process running
pg_sync_replication_slots()
+ * if running. The process will stop upon detecting that the stopSignaled
+ * flag is set to true.

Comment looks slightly odd. Shall we say:

Signal the slotsync worker or the backend process running
pg_sync_replication_slots(), if either one is active. The process...

thanks
Shveta