Hi, On 2026-02-12 11:36:08 +0100, Antonin Houska wrote: > Andres Freund <[email protected]> wrote: > > > For something committable, I think we should probably split IsMVCCSnapshot > > into IsMVCCSnapshot(), just accepting SNAPSHOT_MVCC, and > > IsMVCCLikeSnapshot() > > accepting both SNAPSHOT_MVCC and SNAPSHOT_HISTORIC_MVCC. And then go through > > all the existing callers of IsMVCCSnapshot() - only about half should stay > > as-is, I think. > > The attached patch tries to do that.
Thanks! > From dcdbaf3095e632a1f7f65f3abc43eccff0249d4c Mon Sep 17 00:00:00 2001 > From: Antonin Houska <[email protected]> > Date: Thu, 12 Feb 2026 11:14:00 +0100 > Subject: [PATCH] Refine checking of snapshot type. > > It appears to be confusing if IsMVCCSnapshot() evaluates to true for both > "regular" and "historic" MVCC snapshot. This patch restricts the meaning of > the macro to the "regular" MVCC snapshot, and introduces a new macro > IsMVCCLikeSnapshot() to recognize both types. > > IsMVCCLikeSnapshot() is only used in functions that can (supposedly) be called > during logical decoding. I think I agree with where you selected IsMVCCSnapshot() and where you selected IsMVCCLikeSnapshot(). > diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h > index b8c01a291a1..dd5aaae6953 100644 > --- a/src/include/utils/snapmgr.h > +++ b/src/include/utils/snapmgr.h > @@ -53,12 +53,14 @@ extern PGDLLIMPORT SnapshotData SnapshotToastData; > > /* This macro encodes the knowledge of which snapshots are MVCC-safe */ > #define IsMVCCSnapshot(snapshot) \ > - ((snapshot)->snapshot_type == SNAPSHOT_MVCC || \ > - (snapshot)->snapshot_type == SNAPSHOT_HISTORIC_MVCC) > + ((snapshot)->snapshot_type == SNAPSHOT_MVCC) > > #define IsHistoricMVCCSnapshot(snapshot) \ > ((snapshot)->snapshot_type == SNAPSHOT_HISTORIC_MVCC) > > +#define IsMVCCLikeSnapshot(snapshot) \ > + (IsMVCCSnapshot(snapshot) || IsHistoricMVCCSnapshot(snapshot)) > + > extern Snapshot GetTransactionSnapshot(void); > extern Snapshot GetLatestSnapshot(void); > extern void SnapshotSetCommandId(CommandId curcid); Probably need to update the comments a bit. What about something like /* * Is the snapshot implemented as an MVCC snapshot (i.e. it uses * SNAPSHOT_MVCC). If so, there will be at most be one visible row in a chain * of updated tuples, and each visible tuple will be seen exactly once. */ #define IsMVCCSnapshot(snapshot) \ ... /* * Is the snapshot either an MVCC snapshot or has equivalent visibility * semantics (see IsMVCCSnapshot()). */ #define IsMVCCLikeSnapshot(snapshot) \ Greetings, Andres Freund
