On Fri, 14 Nov 2025 at 14:13, Hayato Kuroda (Fujitsu) <[email protected]> wrote: > > Dear Shlok, > > Thanks for updating the patch. Few more comments. > > > > > > I’m not sure if this has already been discussed; I couldn’t find any > > > > > mention of it in the thread. Why don’t we persist > > > > > 'slot_sync_skip_reason' (it is outside of > > > > > ReplicationSlotPersistentData)? If a slot wasn’t synced during the > > > > > last cycle and the server restarts, it would be helpful to know the > > > > > reason it wasn’t synced prior to the node restart. > > > > > > > Actually I did not think in this direction. I think it will be useful > > to persist 'slot_sync_skip_reason'. I have made the change for the > > same in the latest patch. > > Hmm, I'm wondering it should be written on the disk. Other attributes on the > disk > are essential to decode or replicate changes correctly, but sync status is not > used for the purpose. Personally considered, slot sync would re-start soon > after > the reboot so that it is OK to start with empty. How about others? > > If we want to serialize the info, we should do further tasks: > - update SLOT_VERSION > - make the slot dirty then SaveSlotToPath() when the status is updated. > I agree with your point. Slot synchronization will restart shortly after a reboot, so it seems reasonable to begin with an empty state rather than persisting slot_sync_skip_reason. For now, I’ve updated the patch so that slot_sync_skip_reason is no longer persisted; its initialization is kept outside of ReplicationSlotPersistentData. I’d also like to hear what others think.
> ```
> +static void
> +update_slot_sync_skip_stats(ReplicationSlot *slot, SlotSyncSkipReason
> skip_reason)
> +{
> + Assert(MyReplicationSlot);
> ```
>
> I think no need to require *slot as an argument. We can use the variable to
> shorten
> like update_local_synced_slot().
>
Fixed
> ```
> # Verify pg_sync_replication_slots is failing
> ok( $stderr =~ /skipping slot synchronization because the received slot sync/,
> 'pg_sync_replication_slots failed as expected');
> ```
>
> This may be matter of taste, but can you check whole of log message? Latter
> part
> indicates the actual reason.
>
The latter part of the message contains LSN values, which are not
stable across runs. To avoid hard-coding specific LSNs, I matched the
fixed, non-variable parts of the message while still covering the
reason for the failure.
> ```
> # Detach injection point
> $standby->safe_psql(
> 'postgres', q{
> SELECT injection_points_detach('slot-sync-skip');
> SELECT injection_points_wakeup('slot-sync-skip');
> });
> ```
>
> Not mandatory, but you can quit the background session if you release the
> injection point.
Fixed
Apart from the above changes. I have renamed the functions to
consistently use the term 'slotsync' instead of 'slot_sync'
update_slot_sync_skip_stats -> update_slotsync_skip_stats
pgstat_report_replslot_sync_skip -> pgstat_report_replslotsync_skip
I have attached the updated v8 patch with the latest changes.
Thanks,
Shlok Kyal
v8-0001-Add-stats-related-to-slot-sync-skip.patch
Description: Binary data
v8-0002-Add-test-for-new-stats-for-slot-sync-skip.patch
Description: Binary data
