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

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/