On Fri, Nov 28, 2025 at 5:03 PM Japin Li <[email protected]> wrote: > > 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, >
I don't understand what the comment is here. > 2. > This change seems unnecessary。 > > static void > -slotsync_reread_config(void) > +slotsync_reread_config() > > static void > -ProcessSlotSyncInterrupts(void) > +ProcessSlotSyncInterrupts() > Fixed. > 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); > Fixed. On Fri, Nov 28, 2025 at 5:25 PM shveta malik <[email protected]> wrote: > > Thanks for the patch. Please find a few trivial comments: > > 1) > + if (AmLogicalSlotSyncWorkerProcess()) > + Assert(sync_replication_slots); > > Here too we can use 'worker'. > Fixed. > 2) > + /* check for sync_replication_slots change */ > > check --> Check > Fixed. > 3) > Assert (!worker) > Extra space in between. > Fixed > 4) > check_and_set_sync_info() and ShutDownSlotSync() refers to the pid as > worker_pid. But now it could be backend-pid as well. > Using 'worker' in this variable could be misleading. Shall we make it > sync_process_pid? > Changed. > 5) > /* > * Interrupt handler for main loop of slot sync worker. > */ > static void > ProcessSlotSyncInterrupts() > > We can modify the comment to include API as well. Changed. > > 6) > /* > * Shut down the slot sync worker. > * > * This function sends signal to shutdown slot sync worker, if required. It > * also waits till the slot sync worker has exited or > * pg_sync_replication_slots() has finished. > */ > void > ShutDownSlotSync(void) > > We should change comments to give details on API as well. > changed. > 7) > +# Remove the standby from the synchronized_standby_slots list and reload the > +# configuration. > +$primary->adjust_conf('postgresql.conf', 'synchronized_standby_slots', "''"); > +$primary->reload; > > We can update the comment to below for better clarity: > Remove the dropped sb1_slot from the ... > Changed. > 8) > +# Attempt to synchronize slots using API. The API will continue retrying > +# synchronization until the remote slot catches up. > +# The API will not return until this happens, to be able to make > +# further calls, call the API in a background process. > > We can move these comments atop: > my $h = $standby2->background_psql('postgres', on_error_stop => 0); > Changed. Attached patch v27 addresses the above comments. regards, Ajin Cherian Fujitsu Australia
v27-0001-Improve-initial-slot-synchronization-in-pg_sync_.patch
Description: Binary data
