On Friday, January 30, 2026 2:49 PM Chao Li <[email protected]> wrote:
>
> Thanks for the patch. It’s really an improvement. After reviewing it, I have a
> few small comments.
Thanks for the comments.
>
> 1 - 0001
> ```
> +/*
> + * Helper function to check if the slotsync was skipped and requires re-sync.
> + */
> +static bool
> +should_resync_slot(void)
> +{
> + ReplicationSlot *slot;
> +
> + Assert(MyReplicationSlot);
> +
> + slot = MyReplicationSlot;
> ```
>
> This is a purely a helper function, so I think it doesn’t have to use the
> global
> MyReplicationSlot. We can just pass in a slot, so that this function will be
> more reusable.
Since this is a simple static function which can be used only when the slot is
acquired, I think it's unnecessary to add one parameter since all caller will
pass MyReplicationSlot anyway.
>
> 2 - 0001
> ```
> + * *slotsync_pending is set to true if the slot's synchronization is
> + skipped and
> + * requires re-sync. See should_resync_slot() for cases requiring
> + * re-sync.
> *
> * Returns TRUE if the local slot is updated.
> */
> static bool
> synchronize_one_slot(RemoteSlot *remote_slot, Oid remote_dbid,
> - bool *slot_persistence_pending)
> + bool *slotsync_pending)
> ```
>
> This function only sets *slotsync_pending to true, so it relies on the
> callers to
> initiate it to false. If a caller forgets to initialize it to false, or
> wrongly set it to
> true, then when this function, the variable may contain an unexpected value.
> So I think it’s better to set *slotsync_pending to false in the beginning of
> this
> function.
I think it's the existing behavior before the patch, so I prefer not to change
it for now unless more people suggest it. Besides, setting *slotsync_pending to
false in this function is not sufficient because the synchronize_one_slot()
might not be invoked by synchronize_slots() if no slots need to be synced.
>
> 3 - 0001
> ```
> /* Done if all slots are persisted i.e are sync-ready */
> - if (!slot_persistence_pending)
> + if (!slotsync_pending)
> break;
> ```
>
> I think this comment becomes stale with this patch and needs an update.
> Now it’s only done if persisted and should_resync_slot()==false.
Updated.
>
> 4 - 0002
> ```
> +$primary->adjust_conf('postgresql.conf', 'synchronized_standby_slots',
> +"''"); $primary->reload;
> ```
>
> This reload seems not needed because the next step immediately restarts
> primary.
>
Thanks for catching, it's a copy paste error, fixed.
Best Regards,
Hou zj
