On Fri, 28 Nov 2025 at 15:46, Ajin Cherian <[email protected]> wrote:
> On Wed, Nov 26, 2025 at 7:42 PM shveta malik <[email protected]>
> wrote:
>>
>> A few comments:
>>
>> 1)
>> +/*
>> + * Structure holding parameters that need to be freed on error in
>> + * pg_sync_replication_slots()
>> + */
>> +typedef struct SlotSyncApiFailureParams
>> +{
>> + WalReceiverConn *wrconn;
>> + List *slot_names;
>> +} SlotSyncApiFailureParams;
>> +
>>
>> We can get rid of it now as we do not use it.
>>
>
> Removed.
>
>> 2)
>> ProcessSlotSyncInterrupts():
>>
>> + if (!AmLogicalSlotSyncWorkerProcess())
>> + ereport(ERROR,
>> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>> + errmsg("cannot continue replication slots synchronization"
>> + " as standby promotion is triggered"));
>> + else
>> + {
>>
>> Can we please reverse the if-else i.e. first worker and then API.
>> Negated if-condition can be avoided in this case.
>>
>
> Changed.
>
>> 3)
>>
>> slotsync_reread_config():
>> + /* Worker-specific check for sync_replication_slots change */
>>
>> Now since we check for both API and worker, this comment is not
>> needed.
>>
>
> Removed.
>
>> 4)
>> - ereport(LOG,
>> - errmsg("replication slot synchronization worker will restart
>> because
>> of a parameter change"));
>>
>> - /*
>> - * Reset the last-start time for this worker so that the postmaster
>> - * can restart it without waiting for
>> SLOTSYNC_RESTART_INTERVAL_SEC.
>> - */
>> - SlotSyncCtx->last_start_time = 0;
>> + ereport(AmLogicalSlotSyncWorkerProcess() ? LOG : ERROR,
>> + errmsg("replication slot synchronization will stop because of a
>> parameter change"));
>>
>> Here, we should retain the same old message for worker i.e. 'worker
>> will restart..'. instead of 'synchronization will stop'. I find the
>> old message better in this case.
>>
>> 5)
>> slotsync_reread_config() is slightly difficult to follow.
>> I think in the case of API, we can display a common error message
>> instead of 2 different messages for 'sync_replication_slot change'
>> and
>> the rest of the parameters. We can mark if any of the parameters
>> changed in both 'if' blocks and if the current process has not
>> exited,
>> then at the end based on 'parameter-changed', we can deal with API
>> by
>> giving a common message. Something like:
>>
>> /*
>> * 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 (new variable))
>> {
>> Assert (!AmLogicalSlotSyncWorkerProcess);
>> ereport(ERROR,
>> errmsg("replication slot synchronization will stop because of a
>> parameter change"));
>> }
>>
>
> Fixed as above.
>
> I've addressed the above comments as well as rebased the patch based
> on changes in commit 76b7872 in patch v26
>
1.
Initialize slot_persistence_pending to false (to avoid uninitialized values, or
initialize to true by mistaken) in update_and_persist_local_synced_slot(). This
aligns with the handling of found_consistent_snapshot and remote_slot_precedes
in update_local_synced_slot().
diff --git a/src/backend/replication/logical/slotsync.c
b/src/backend/replication/logical/slotsync.c
index 20eada3393..c55ba11f17 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -617,6 +617,9 @@ update_and_persist_local_synced_slot(RemoteSlot
*remote_slot, Oid remote_dbid,
bool found_consistent_snapshot = false;
bool remote_slot_precedes = false;
+ if (slot_persistence_pending)
+ *slot_persistence_pending = false;
+
/* Slotsync skip stats are handled in function
update_local_synced_slot() */
(void) update_local_synced_slot(remote_slot, remote_dbid,
&found_consistent_snapshot,
2.
This change seems unnecessary。
static void
-slotsync_reread_config(void)
+slotsync_reread_config()
static void
-ProcessSlotSyncInterrupts(void)
+ProcessSlotSyncInterrupts()
3.
Since we are already caching the result of AmLogicalSlotSyncWorkerProcess() in
a local worker variable, how about applying this replacement:
s/if (AmLogicalSlotSyncWorkerProcess())/if (worker)/g?
+ bool worker = AmLogicalSlotSyncWorkerProcess();
+ bool parameter_changed = false;
- Assert(sync_replication_slots);
+ if (AmLogicalSlotSyncWorkerProcess())
+ Assert(sync_replication_slots);
--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
