Thread

Re: A few patches to clarify snapshot management, part 2

Andres Freund <andres@anarazel.de> — 2025-12-19T15:16:36Z

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