Обсуждение: 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

> On Dec 19, 2025, at 19:51, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
>> +/*
>> + * 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
alwaysavailable. 

Oh, I didn’t notice each of the structs contain “snapshot_type”, in that case, maybe we can just define SnapshotData as
astructure with a single field “snapshot_type”, in the same way as “Node”. 

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

On 19/12/2025 17:16, Andres Freund wrote:
> 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

Ok. (Chao suggested that too)

> - I'd rename the fields for dirty snapshots, given how differently they're
>    used for dirty snapshots, compared to anything else

Hmm. The downside is a little more code churn, also in extensions that 
use DirtySnapshot. There are only few of them though, based on quick 
grepping I believe only pglogical is affected, so I think you're right. 
I renamed them to 'updating_xmin' and 'updating_xmax'.

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

Rats! Historic MVCC snapshots can indeed be pushed to the active stack. 
There is no core code that does that, but pglogical does it. This is 
precisely the case that Noah pointed out earlier [1], when I added an 
assertion against that in GetTransactionSnapshot(), and had to revert it.

Ok so we need to keep the support for that. That complicates the first 
patches in this series a little, but it's not too bad.

[1] 
https://www.postgresql.org/message-id/20250809222338.cc.nmisch%40google.com

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

It doesn't catch everything, but it catches more misuse than without it. 
For example, this would go unnoticed on 'master', but would hit the new 
assertion on the 'valid' flag:

snapshot = GetTransactionSnapshot();
PushActiveSnapshot(snapshot);

/* this invalidates the 'snapshot' and advances the advertised xmin */
PopActiveSnapshot();

/* This is wrong! 'snapshot' is invalid */
PushActiveSnapshot(snapshot);


But this mistake goes unnoticed with or without these patches:

snapshot = GetTransactionSnapshot();
PushActiveSnapshot(snapshot);

/* this invalidates the 'snapshot' and advances the advertised xmin */
PopActiveSnapshot();

other_snapshot = GetTransactionSnapshot();

/*
  * This "works", because 'snapshot' and 'other_snapshot' both point to
  * the same static SnapshotData. But it makes the *new* snapshot active,
  * not the first one!
  */
PushActiveSnapshot(snapshot);



Here's a new patch set. I only included the more mature patches for now, 
the rest need to be rebased.

The first patch is new. It adds sanity checks to a few places that 
previously assumed that they were dealing with a regular MVCC snapshot 
without checking.

Patch 0002 now accepts historic MVCC snapshots in PushActiveSnapshot(). 
I tested that it works with pglogical now, after updating pglogical 
renamed fields and such.

- Heikki

Hello,

Please have a look at patch 0003 here:
https://postgr.es/m/210036.1765651719@localhost
maybe it would be useful to put that change in while you're hacking on
this code, to avoid having to change it again soon after.  It's quite
small.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On 2025-Dec-22, Heikki Linnakangas wrote:

> @@ -146,7 +146,11 @@ find_inheritance_children_extended(Oid parentrelId, bool omit_detached,
>                  Snapshot    snap;
>  
>                  xmin = HeapTupleHeaderGetXmin(inheritsTuple->t_data);
> +
> +                /* XXX: what should we do with a non-MVCC snapshot? */
>                  snap = GetActiveSnapshot();
> +                if (snap->snapshot_type != SNAPSHOT_MVCC)
> +                    elog(ERROR, "cannot look up partition information with a non-MVCC snapshot");
>  
>                  if (!XidInMVCCSnapshot(xmin, snap))
>                  {

I think this code should be skipped when a non-MVCC snapshot is in use.
I assume you won't hit the case unless you test for it specifically, so
I'm not surprised that just adding an elog(ERROR) does not cause visible
failures; however in production it probably would.  And looking up
partition info in that case is, most likely, useless.  So changing the
'if' test 
        if (!XidInMVCCSnapshot(xmin, snap))
to only do this dance if "->snapshot_type == SNAPSHOT_MVCC" seems
reasonable to me.

I'm not sure what's a good way to verify this, however.  We would need a
test that adds partitions pending detach to partitioned tables used by
as many other tests as possible ...  (Maybe: have test_setup.sql add an
event trigger that runs on CREATE TABLE, detects PARTITION BY, and
creates and partially detaches a partition.  Not sure how feasible this
is.)

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On 05/01/2026 19:01, Álvaro Herrera wrote:
> On 2025-Dec-22, Heikki Linnakangas wrote:
> 
>> @@ -146,7 +146,11 @@ find_inheritance_children_extended(Oid parentrelId, bool omit_detached,
>>                   Snapshot    snap;
>>   
>>                   xmin = HeapTupleHeaderGetXmin(inheritsTuple->t_data);
>> +
>> +                /* XXX: what should we do with a non-MVCC snapshot? */
>>                   snap = GetActiveSnapshot();
>> +                if (snap->snapshot_type != SNAPSHOT_MVCC)
>> +                    elog(ERROR, "cannot look up partition information with a non-MVCC snapshot");
>>   
>>                   if (!XidInMVCCSnapshot(xmin, snap))
>>                   {
> 
> I think this code should be skipped when a non-MVCC snapshot is in use.
> I assume you won't hit the case unless you test for it specifically, so
> I'm not surprised that just adding an elog(ERROR) does not cause visible
> failures; however in production it probably would.  And looking up
> partition info in that case is, most likely, useless.  So changing the
> 'if' test
>         if (!XidInMVCCSnapshot(xmin, snap))
> to only do this dance if "->snapshot_type == SNAPSHOT_MVCC" seems
> reasonable to me.

Yeah, I suppose the most reasonable behavior is to treat a non-MVCC 
snapshot the same as no active snapshot at all here. Although I'm not 
convinced that this is reachable in any sensible scenario (see below).

> I'm not sure what's a good way to verify this, however.  We would need a
> test that adds partitions pending detach to partitioned tables used by
> as many other tests as possible ...  (Maybe: have test_setup.sql add an
> event trigger that runs on CREATE TABLE, detects PARTITION BY, and
> creates and partially detaches a partition.  Not sure how feasible this
> is.)

None of the built-in code pushes a historic snapshot to the active 
stack, so that won't find anything. The only known instance of that is 
pglogical's row filters. Is it sane for a row filter, or other 
hypothetical expression that is executed during logical decoding, to 
access a partitioned table?

We don't allow marking a partitioned table as a "user catalog table":

postgres=# CREATE TABLE parttab (i int) PARTITION BY RANGE (i);
CREATE TABLE
postgres=# ALTER TABLE parttab SET (user_catalog_table = true);
ERROR:  cannot specify storage parameters for a partitioned table
HINT:  Specify storage parameters for its leaf partitions instead.

And it's not sane to access tables other than catalog tables with a 
historic snapshot. So I'm not sure this reachable in any sensible scenario.

- Heikki