Re: A few patches to clarify snapshot management, part 2 - Mailing list pgsql-hackers

> On Dec 19, 2025, at 07:30, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> This is a continuation of the earlier thread with the same subject [1], and related to the CSN work [2].
>
> I'm pretty happy patches 0001-0005. They make the snapshot management more clear in many ways:
>
> Patch 0001: Use a proper type for RestoreTransactionSnapshot's PGPROC arg
>
> Minor cleanup, independent of the rest of the patches
>

Looks like this cleanup should have done earlier. The old comments says that only reason to use void * is to avoid
includea header file. But in snapmgr.h, there already has a usage of “struct HTAB” from 12 years ago. 

Maybe we can also do:
```
typedef struct PGPROC PGPROC;
extern void RestoreTransactionSnapshot(MVCCSnapshot snapshot, PGPROC *source_pgproc);
```

So the function declaration looks neater. snapmgr.h line 101 has done in this way. If not pretty much burden, we can
updateHTAB as well. 

I believe 0001 is an independent self-contained commit that can be pushed first.

>
> 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
whichfields are used with which kind of snapshot. For example, we now have properly named fields for the XID arrays in
historicsnapshots. Previously, they abused the 'xip' and 'subxip' arrays to mean something different than what they
meanin regular MVCC snapshots. 
>
> This introduces some new casts between Snapshots and the new MVCCSnapshots. I struggled to decide which functions
shoulduse the new MVCCSnapshot type and which should continue to use Snapshot. It's a balancing act between code churn
andwhere we want to have casts. For example, PushActiveSnapshot() takes a Snapshot argument, but assumes that it's an
MVCCsnapshot, so it really should take MVCCSnapshot. But most of its callers have a Snapshot rather than MVCCSnapshot.
Anotherexample: the executor assumes that it's dealing with MVCC snapshots, so it would be appropriate to use
MVCCSnapshotfor EState->es_snapshot. But it'd cause a lot code churn and casts elsewhere. I think the patch is a
reasonablemiddle ground. 
>

The core of 0002 is the new union Snapshot:
```
+/*
+ * Generic union representing all kind of possible snapshots.  Some have
+ * type-specific structs.
+ */
+typedef union SnapshotData
+{
+    SnapshotType snapshot_type; /* type of snapshot */
+
+    MVCCSnapshotData mvcc;
+    DirtySnapshotData dirty;
+    HistoricMVCCSnapshotData historic_mvcc;
+    NonVacuumableSnapshotData nonvacuumable;
 } SnapshotData;
```

And my big concern is here. This union definition looks unusual, snapshot_type shares the same space with real snapshot
bodies,so that once snapshot is assigned to the union, that type info is lost, there would be no way to decide what
exactsnapshot is stored in SnapshotData. 

I think SnapshotData should be a structure and utilize anonymous union technique:
```
typedef struct SnapshotData
{
    SnapshotType snapshot_type; /* type of snapshot */

    union {
       MVCCSnapshotData mvcc;
       DirtySnapshotData dirty;
       HistoricMVCCSnapshotData historic_mvcc;
       NonVacuumableSnapshotData nonvacuumable;
    }
```

If you agree with this, then 0002 will need some rework. A set of helper micros/functions would need to be added, e.g.
InitDirtySnapshot(),DirtySnapshotGetSnapshot()... 

So that, I’d stop reviewing, and see what do you think.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/