> On Dec 10, 2025, at 10:40, Ajin Cherian <[email protected]> wrote:
>
> On Wed, Dec 10, 2025 at 1:29 PM Chao Li <[email protected]> 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/
