On 16/12/2024 23:56, Nathan Bossart wrote:
On Mon, Dec 16, 2024 at 12:06:33PM +0200, Heikki Linnakangas wrote:
While working on the CSN snapshot patch, I got sidetracked looking closer
into the snapshot tracking in snapmgr.c. Attached are a few patches to
clarify some things.
I haven't yet looked closely at what you are proposing, but big +1 from me
for the general idea. I recently found myself wishing for a lot more
commentary about this stuff [0].
[0] https://postgr.es/m/Z0dB1ld2iPcS6nC9%40nathan
While playing around some more with this, I noticed that this code in
GetTransactionSnapshot() is never reached, and AFAICS has always been
dead code:
Snapshot
GetTransactionSnapshot(void)
{
/*
* Return historic snapshot if doing logical decoding. We'll never need
a
* non-historic transaction snapshot in this (sub-)transaction, so
there's
* no need to be careful to set one up for later calls to
* GetTransactionSnapshot().
*/
if (HistoricSnapshotActive())
{
Assert(!FirstSnapshotSet);
return HistoricSnapshot;
}
when you think about it, that's good, because it doesn't really make
sense to call GetTransactionSnapshot() during logical decoding. We jump
through hoops to make the historic catalog decoding possible with
historic snapshots, tracking subtransactions that modify catalogs and
WAL-logging command ids, but they're not suitable for general purpose
queries. So I think we should turn that into an error, per attached patch.
Another observation is that we only ever use regular MVCC snapshots as
active snapshots. I added a "Assert(snapshot->snapshot_type ==
SNAPSHOT_MVCC);" to PushActiveSnapshotWithLevel() and all regression
tests passed. That's also good, because we assumed that much in a few
places anyway: there are a couple of calls that amount to
"XidInMVCCSnapshot(..., GetActiveSnapshot()"), in
find_inheritance_children_extended() and RelationGetPartitionDesc(). We
could add comments and that assertion to make that assumption explicit.
And that thought takes me deeper down the rabbit hole:
/*
* Struct representing all kind of possible snapshots.
*
* There are several different kinds of snapshots:
* * Normal MVCC snapshots
* * MVCC snapshots taken during recovery (in Hot-Standby mode)
* * Historic MVCC snapshots used during logical decoding
* * snapshots passed to HeapTupleSatisfiesDirty()
* * snapshots passed to HeapTupleSatisfiesNonVacuumable()
* * snapshots used for SatisfiesAny, Toast, Self where no members are
* accessed.
*
* TODO: It's probably a good idea to split this struct using a NodeTag
* similar to how parser and executor nodes are handled, with one type for
* each different kind of snapshot to avoid overloading the meaning of
* individual fields.
*/
typedef struct SnapshotData
I'm thinking of implementing that TODO, splitting SnapshotData into
separate structs like MVCCSnapshotData, SnapshotDirtyData, etc. It seems
to me most places can assume that you're dealing with MVCC snapshots,
and if we had separate types for them, could be using MVCCSnapshot
instead of the generic Snapshot. Only the table and index AM functions
need to deal with non-MVCC snapshots.
--
Heikki Linnakangas
Neon (https://neon.tech)
From ec248c69cb42a0747ecc6a63ac4e4682cce2ee93 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Fri, 20 Dec 2024 18:37:44 +0200
Subject: [PATCH 1/1] Don't allow GetTransactionSnapshot() in logical decoding
A historic snapshot should only be used for catalog access, not
general queries. We never call GetTransactionSnapshot() during logical
decoding, which is good because it wouldn't be very sensible, so the
code to deal with that was unreachable and untested. Turn it into an
error, to avoid doing that in the future either.
---
src/backend/utils/time/snapmgr.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index e60360338d5..3717869f736 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -212,16 +212,12 @@ Snapshot
GetTransactionSnapshot(void)
{
/*
- * Return historic snapshot if doing logical decoding. We'll never need a
- * non-historic transaction snapshot in this (sub-)transaction, so there's
- * no need to be careful to set one up for later calls to
- * GetTransactionSnapshot().
+ * This should not be called while doing logical decoding. Historic
+ * snapshots are only usable for catalog access, not for general-purpose
+ * queries.
*/
if (HistoricSnapshotActive())
- {
- Assert(!FirstSnapshotSet);
- return HistoricSnapshot;
- }
+ elog(ERROR, "cannot take query snapshot during logical decoding");
/* First call in transaction? */
if (!FirstSnapshotSet)
--
2.39.5
