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

On Fri, Nov 28, 2025 at 5:03 PM Japin Li <japinli@hotmail.com> 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 <shveta.malik@gmail.com> 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