On Tuesday, February 10, 2026 3:02 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Mon, Feb 9, 2026 at 11:36 AM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > Adjusted the comments as suggested.
> >
> > In addition to addressing the comments, I revisited the recently
> > updated slotsync code and noticed opportunities to simplify some
> > parameters, checks, and codes. This will also facilitate the improvement in
> v2-0001 coding.
> >
> > * Previously, certain function parameters(found_consistent_snapshot,
> > remote_slot_precedes of update_local_synced_slot()) were used to store
> > the reason for slot synchronization being skipped. However, now that a
> > slot property serves this purpose, we can simplify the code by
> > eliminating those redundant parameters and using the slot's property to
> perform the same check.
> >
> > * The slot synchronization is skipped if the required WAL has not been
> > received and flushed. Previously, this check[1] was performed in two
> separate code paths.
> > Such duplication can lead to coding errors if changes are made in one
> > location without updating the other, as exemplified by the issue fixed in
> commit 3df4df5.
> > This commit consolidates the check into a single location to eliminate
> > redundancies and reduce the potential for future errors.
> >
> > To address these points, I have created two additional patches:
> > V3-0001 for the first point and V3-0002 for the second. V3-0003
> > contains the current improvement being discussed, and it's also simplified
> thanks to the preceding patches.
> >
> I like the idea of both the new patches. Please find a few trivial comments:
Thanks for the comments.
>
> patch002:
> 1)
> Earlier at both the places where we were updating
> 'SS_SKIP_WAL_NOT_FLUSHED', we were returning slot_updated as false,
> now, we might end up returning it as true (specially at second occurrence). Is
> this intentional?
Yes. I think it's OK in the second occurrence because we did create a new temp
slot and give some initial value for the slot. I think it's similar to
SS_SKIP_WAL_OR_ROWS_REMOVED and SS_SKIP_NO_CONSISTENT_SNAPSHOT where we also
return slot_updated=true in case of initial sync.
>
> 2)
> In update_and_persist_local_synced_slot(), we can reach this even when
> wal_not_flushed, so we shall to update comment:
> if (slot->slotsync_skip_reason != SS_SKIP_NONE)
> {
> /*
> * We reach here when the remote slot didn't catch up to locally
> * reserved position, or it cannot reach the consistent point from the
> * restart_lsn.
> ....
> */
Updated.
>
> Patch003:
> 3)
> + if (slotsync_pending && slot->slotsync_skip_reason != SS_SKIP_NONE)
> + *slotsync_pending = true;
>
> Here shall we ensure by a sanity check that slotsync_skip_reason !=
> SS_SKIP_INVALID?
Added an Assert for it.
> And please bring back the comment as well, which was
> there in an earlier patch which stated the reason for not including
> 'SS_SKIP_INVALID' here.
After rethinking, I chose to add the comments atop of file
along with other related comments.
Best Regards,
Hou zj
