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

> 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 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/