Re: Improve pg_sync_replication_slots() to wait for primary to advance

> On Dec 10, 2025, at 10:40, Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Wed, Dec 10, 2025 at 1:29 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>>
>> Hi Ajin,
>>
>> I’d like to revisit this patch, but looks like 04396eacd3faeaa4fa3d084a6749e4e384bdf0db has some conflicts to this patch. So can you please rebase this patch?
>>
>> Best regards,
>> --
>
> It's been rebased. Have a look at the latest version.
>

Here are some comments for v32.

1 - 0001
```
- * The slot sync worker's pid is needed by the startup process to shut it
- * down during promotion. The startup process shuts down the slot sync worker
- * and also sets stopSignaled=true to handle the race condition when the
+ * The pid is either the slot sync worker's pid or the backend's pid running
```

I think we should add single quotes for “pid” and “stopSignaled". Looking at other comment lines, structure fields all have single-quoted:
```
* The 'syncing' flag is needed to prevent concurrent slot syncs to avoid slot

* The 'last_start_time' is needed by postmaster to start the slot sync worker
```

2 - 0001 - the same code block as 1

I wonder how to distinct if the “pid” is a slot sync worker or a backend process?

3 - 0001
```
+ bool worker = AmLogicalSlotSyncWorkerProcess();
```

The variable name “worker” doesn’t indicate a bool type, maybe renamed to “is_slotsync_worker”.

4 - 0001
```
+ /*
+ * If we have reached here with a parameter change, we must be running in
+ * SQL function, emit error in such a case.
+ */
+ if (parameter_changed)
+ {
+ Assert(!worker);
+ ereport(ERROR,
+ errmsg("replication slot synchronization will stop because of a parameter change"));
}
```

The Assert(!worker) feels redundant, because it then immediately error out.

5 - 0001
```
+ * Exit or throw errors if relevant GUCs have changed depending on whether
+ * called from slotsync worker or from SQL function pg_sync_replication_slots()
```

Let’s change “slotsync” to “slot sync” because elsewhere comments all use “slot sync”, just to keep consistent.

6 - 0001
```
- * Interrupt handler for main loop of slot sync worker.
+ * Interrupt handler for main loop of slot sync worker and
+ * SQL function pg_sync_replication_slots().
```

Missing “the” before “SQL function”. This comment applies to multiple places.

7 - 0001
```
+ if (sync_process_pid!= InvalidPid)
+ kill(sync_process_pid, SIGUSR1);
```

Nit: missing a white space before “!="

8 - 0002
```
+ if (slot_names != NIL)
{
- StartTransactionCommand();
- started_tx = true;
+ bool first_slot = true;
+
+ /*
+ * Construct the query to fetch only the specified slots
+ */
+ appendStringInfoString(&query, " AND slot_name IN (");
+
+ foreach_ptr(char, slot_name, slot_names)
+ {
+ if (!first_slot)
+ appendStringInfoString(&query, ", ");
+
+ appendStringInfo(&query, "'%s'", slot_name);
+ first_slot = false;
+ }
+ appendStringInfoChar(&query, ')');
}
```

The logic of appending “, “ can be slightly simplified as:
```
If (slot_names != NIL)
{
Const char *sep = “”;
appendStringInfoString(&query, " AND slot_name IN (“);
foreach_ptr(char, slot_name, slot_names)
{
appendStringInfo(&query, “%s'%s'", sep, slot_name);
sep = “, “;
}
}
```

That saves a “if” check and a appendStringInfoString().

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/