Hi, On 2025-12-19 01:30:05 +0200, Heikki Linnakangas wrote: > Patch 0002: Split SnapshotData into separate structs for each kind of > snapshot > > This implements the long-standing TODO and splits SnapshotData up into > multiple structs. This makes it more clear which fields are used with which > kind of snapshot. For example, we now have properly named fields for the XID > arrays in historic snapshots. Previously, they abused the 'xip' and 'subxip' > arrays to mean something different than what they mean in regular MVCC > snapshots. > > This introduces some new casts between Snapshots and the new MVCCSnapshots. > I struggled to decide which functions should use the new MVCCSnapshot type > and which should continue to use Snapshot. It's a balancing act between code > churn and where we want to have casts. For example, PushActiveSnapshot() > takes a Snapshot argument, but assumes that it's an MVCC snapshot, so it > really should take MVCCSnapshot. But most of its callers have a Snapshot > rather than MVCCSnapshot. Another example: the executor assumes that it's > dealing with MVCC snapshots, so it would be appropriate to use MVCCSnapshot > for EState->es_snapshot. But it'd cause a lot code churn and casts > elsewhere. I think the patch is a reasonable middle ground.
Generally I think this is good. A few points: - I'd use a SnapshotBase type or such that then has the SnapshotType as a member. That way we can more cleanly add common fields if we need them - I'd rename the fields for dirty snapshots, given how differently they're used for dirty snapshots, compared to anything else - I'm somewhat doubtful that it's rigth to restict active snapshots to plain mvcc ones. Why is that the right thing? What if a historic mvcc snapshot is supposed to be pushed? What if we introduce different types of snapshots in the future, e.g. CSN ones on the standby? > Patch 0004: Add an explicit 'valid' flag to MVCCSnapshotData > > Makes it more clear when the "static" snapshots returned by > GetTransactionSnapshot(), GetLatestSnapshot() and GetCatalogSnapshot() are > valid. Given they're pointing to static memory location, won't that limit how much we can rely on valid? If something else builds a snapshot a "wrong" reference to a snapshot will suddenly appear valid again, no? Greetings, Andres Freund
