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

> On Dec 10, 2025, at 10:40, Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Wed, Dec 10, 2025 at 1:29 PM Chao Li <li.evan.chao@gmail.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
havesingle-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/