Horton

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

shveta malik <shveta.malik@gmail.com>

View thread
From: shveta malik <shveta.malik@gmail.com>
To: Ajin Cherian <itsajin@gmail.com>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, Japin Li <japinli@hotmail.com>, Ashutosh Sharma <ashu.coek88@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, PostgreSQL mailing lists <pgsql-hackers@postgresql.org>, shveta malik <shveta.malik@gmail.com>
Date: 2025-11-21T09:10:14Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Enhance slot synchronization API to respect promotion signal.

  2. Fix inconsistent elevel in pg_sync_replication_slots() retry logic.

  3. Refactor slot synchronization logic in slotsync.c.

  4. Fix intermittent BF failure in 040_standby_failover_slots_sync.

  5. Add retry logic to pg_sync_replication_slots().

  6. Fix LOCK_TIMEOUT handling in slotsync worker.

  7. Add slotsync skip statistics.

On Fri, Nov 21, 2025 at 9:14 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
>
> Attaching patch v24, addressing the above comments.
>

Thanks for the patch. Please find a few comments:


1)
Instead of passing an argument to slotsync_reread_config and
ProcessSlotSyncInterrupts, we can use 'AmLogicalSlotSyncWorkerProcess'
to distinguish the worker and API.

2)
Also, since we are not using a separate memory context, we don't need
to make structure 'SlotSyncApiFailureParams' for slot_names failure.
slot_names will be freed with the memory-context itself when
exec_simple_query finishes.

3)
- if (old_sync_replication_slots != sync_replication_slots)
+ /* Worker-specific check for sync_replication_slots change */
+ if (!from_api && old_sync_replication_slots != sync_replication_slots)
  {
  ereport(LOG,
- /* translator: %s is a GUC variable name */
- errmsg("replication slot synchronization worker will shut down
because \"%s\" is disabled", "sync_replication_slots"));
+ /* translator: %s is a GUC variable name */
+ errmsg("replication slot synchronization worker will shut down
because \"%s\" is disabled",
+    "sync_replication_slots"));
  proc_exit(0);
  }

Here, we need not to have different flow for api and worker. Both can
quit sync when this parameter is changed. The idea is if someone
enables 'sync_replication_slots' when API is working, that means we
need to start slot-sync worker, so it is okay if the API notices this
and exits too.

4)
+ if (from_api)
+ elevel = ERROR;
+ else
+ elevel = LOG;

- proc_exit(0);
+ ereport(elevel,
+ errmsg("replication slot synchronization will stop because of a
parameter change"));
+

We can do:
ereport(AmLogicalSlotSyncWorkerProcess() ? LOG : ERROR, ...);

5)
 SlotSyncCtx->syncing = true;

+ /* The worker pid must not be already assigned in SlotSyncCtx */
+ Assert(SlotSyncCtx->pid == InvalidPid);
+

We can shift Assert before we set the shared-memory flag 'SlotSyncCtx->syncing'.

thanks
Shveta