On Fri, Nov 13, 2026 at 7:29 PM Bertrand Drouvot <[email protected]> wrote: > > On Fri, Nov 13, 2026 at 04:45:00PM +0530, Amit Kapila wrote: > > > > + else if (slot->data.persistency == RS_TEMPORARY) > > + { > > + /* > > + * We reach this point when a previous cycle updated confirmed_flush > > + * but failed to build a consistent snapshot, and the remote slot has > > + * not advanced beyond the local slot since then. > > + */ > > + skip_reason = SS_SKIP_NO_CONSISTENT_SNAPSHOT; > > + } > > > > Is it better to directly use the slot's skip_reason rather than > > assuming SS_SKIP_NO_CONSISTENT_SNAPSHOT? > > That could work but what about the case I mentioned upthread where the local > catalog_xmin gets ahead and the remote later catches up? I think that that > would > keep slotsync_skip_reason as SS_SKIP_WAL_OR_ROWS_REMOVED, although the slot is > still inconsistent due to the missing snapshot. >
Right the reason can remain as SS_SKIP_WAL_OR_ROWS_REMOVED. Even in the first cycle, the reason could be either SS_SKIP_WAL_NOT_FLUSHED or SS_SKIP_WAL_OR_ROWS_REMOVED and in that case also the caller update_and_persist_local_synced_slot() won't persist the slot. So, I don't see a problem with retaining the previous reason when we didn't perform anything in a particular sync cycle. > > I understand today it is correct but tomorrow if there are more reasons > > Maybe we could keep v2 and assert the two possible previous reasons, so any > new > reason would need explicit handling? > Hmm, yeah that kind of forceful handling can also work but when we don't see any harm in retaining the previous skip reason then why not do that instead? -- With Regards, Amit Kapila.
