Обсуждение: A few patches to clarify snapshot management, part 2

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


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.


Patch 0003: Simplify historic snapshot refcounting

Little refactoring in logical decoding's snapshot tracking.


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.


Patch 0005: Replace static snapshot pointers with the 'valid' flags

Refactor snapmgr.c a little, taking advantage of the new 'valid' flags


The rest of the patches are less polished, but please take a look. The 
idea is to split MVCCSnapshot further into a "shared" part that contains 
the xmin, xmax and the 'xip' array, and an outer shell that contains the 
mutable 'curcid' field. This allows reusing the shared part in more 
cases, avoiding copying, although I'm not sure what would be a practical 
scenario where it'd make a performance difference. It becomes more 
important with the CSN patch, however, because it adds a cache to the 
shared snapshot struct, which can potentially grow very large.

[1] 
https://www.postgresql.org/message-id/7c56f180-b9e1-481e-8c1d-efa63de3ecbb%40iki.fi
[2] 
https://www.postgresql.org/message-id/80f254d3-8ee9-4cde-a7e3-ee99998154da%40iki.fi

- Heikki

> 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/

On 19/12/2025 07:15, Chao Li wrote:
>> On Dec 19, 2025, at 07:30, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> 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.
 

Yeah, we're not very consistent about that. I followed the example of 
HTAB and went with "struct PGPROC" for now.

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

Pushed 0001, thanks for the review!

- Heikki

On 19/12/2025 07:15, Chao Li wrote:
>> On Dec 19, 2025, at 07:30, Heikki Linnakangas <hlinnaka@iki.fi> 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.
> > 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 exact snapshot is stored in
> SnapshotData.

Each of the structs, MVCCSnapshotData, DirtySnapshotData, etc., contain 
'snapshot_type' as the first field, so it's always available.

> 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;
>      }
> ```

Hmm, yeah, maybe that would be more clear. But then you cannot cast an 
"MVCCSnapshotData *" to "SnapshotData *", or vice versa.

> 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()...
 

Right, I feel those helper macros / functions would be excessive.

- Heikki

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

On Fri, 19 Dec 2025 at 16:51, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 19/12/2025 07:15, Chao Li wrote:
> >> On Dec 19, 2025, at 07:30, Heikki Linnakangas <hlinnaka@iki.fi> 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.
> > > 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 exact snapshot is stored in
> > SnapshotData.
>
> Each of the structs, MVCCSnapshotData, DirtySnapshotData, etc., contain
> 'snapshot_type' as the first field, so it's always available.
>
> > 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;
> >      }
> > ```
>
> Hmm, yeah, maybe that would be more clear. But then you cannot cast an
> "MVCCSnapshotData *" to "SnapshotData *", or vice versa.
>
> > 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()...
 
>
> Right, I feel those helper macros / functions would be excessive.
>
> - Heikki
>
>
>


Hi! I have been looking through this series of patches, when I noticed
that UnregisterSnapshotFromOwner/UnregisterSnapshot use this coding:

```
if (snapshot == InvalidSnapshot)
return;
```

First of all, I am not sure this is the type of check we usually do in
core. If this coding practice is OK, should we remove this check from
UnregisterSnapshot* caller?
Like in PFA.

Sorry for bikeshedding

-- 
Best regards,
Kirill Reshke