On 04/12/2024 03:24, Tom Lane wrote:
Andres Freund <and...@anarazel.de> writes:
On 2024-12-03 22:06:59 +0200, Heikki Linnakangas wrote:
I spotted some more remnants of the "snapshot too old" feature that was
removed in v17. Barring objections, I will commit the attached patch to tidy
up.
Most of this I agree with. But I'm not sure just removing the toast snapshot
stuff is good - we've had a bunch of bugs where we don't hold a snapshot for
long enough to actually ensure that toast tuples stay alive.
Yeah, the stuff concerned with toast snapshots has nothing to do
with that old user-visible feature. It's to keep us from writing
incorrect code, and it's still (very) needed.
Right. Here's a new attempt that keeps that check.
--
Heikki Linnakangas
Neon (https://neon.tech)
From 8ec645a1182969feb95c15db846e4b294d2a40dd Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Wed, 4 Dec 2024 09:52:41 +0200
Subject: [PATCH v2 1/1] Remove remants of "snapshot too old"
Remove the 'whenTaken' and 'lsn' fields from SnapshotData. After the
removal of the "snapshot too old" feature, they were never set to a
non-zero value.
This largely revert commit 3e2f3c2e423, which added the
OldestActiveSnapshot tracking, and the init_toast_snapshot()
function. That was only required for setting the 'whenTaken' and 'lsn'
fields. SnapshotToast is now a constant again, like SnapshotSelf and
SnapshotAny. I kept a thin get_toast_snapshot() wrapper around
SnapshotToast though, to check that you have a registered or active
snapshot. That's still a useful sanity check.
Reviewed-by: Nathan Bossart, Andres Freund, Tom Lane
Discussion: https://www.postgresql.org/message-id/cd4b4f8c-e63a-41c0-95f6-6e6cd9b83...@iki.fi
---
contrib/amcheck/verify_heapam.c | 4 +-
src/backend/access/common/toast_internals.c | 51 ++++++++-------------
src/backend/access/heap/heaptoast.c | 4 +-
src/backend/storage/ipc/procarray.c | 4 --
src/backend/utils/time/snapmgr.c | 47 +------------------
src/include/access/toast_internals.h | 2 +-
src/include/utils/snapmgr.h | 13 ++----
src/include/utils/snapshot.h | 3 --
8 files changed, 25 insertions(+), 103 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index 9c74daacee..e16557aca3 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -1767,7 +1767,6 @@ check_tuple_attribute(HeapCheckContext *ctx)
static void
check_toasted_attribute(HeapCheckContext *ctx, ToastedAttribute *ta)
{
- SnapshotData SnapshotToast;
ScanKeyData toastkey;
SysScanDesc toastscan;
bool found_toasttup;
@@ -1791,10 +1790,9 @@ check_toasted_attribute(HeapCheckContext *ctx, ToastedAttribute *ta)
* Check if any chunks for this toasted object exist in the toast table,
* accessible via the index.
*/
- init_toast_snapshot(&SnapshotToast);
toastscan = systable_beginscan_ordered(ctx->toast_rel,
ctx->valid_toast_index,
- &SnapshotToast, 1,
+ get_toast_snapshot(), 1,
&toastkey);
found_toasttup = false;
while ((toasttup =
diff --git a/src/backend/access/common/toast_internals.c b/src/backend/access/common/toast_internals.c
index 90d0654e62..1939cfb4d2 100644
--- a/src/backend/access/common/toast_internals.c
+++ b/src/backend/access/common/toast_internals.c
@@ -393,7 +393,6 @@ toast_delete_datum(Relation rel, Datum value, bool is_speculative)
HeapTuple toasttup;
int num_indexes;
int validIndex;
- SnapshotData SnapshotToast;
if (!VARATT_IS_EXTERNAL_ONDISK(attr))
return;
@@ -425,9 +424,8 @@ toast_delete_datum(Relation rel, Datum value, bool is_speculative)
* sequence or not, but since we've already locked the index we might as
* well use systable_beginscan_ordered.)
*/
- init_toast_snapshot(&SnapshotToast);
toastscan = systable_beginscan_ordered(toastrel, toastidxs[validIndex],
- &SnapshotToast, 1, &toastkey);
+ get_toast_snapshot(), 1, &toastkey);
while ((toasttup = systable_getnext_ordered(toastscan, ForwardScanDirection)) != NULL)
{
/*
@@ -631,41 +629,28 @@ toast_close_indexes(Relation *toastidxs, int num_indexes, LOCKMODE lock)
}
/* ----------
- * init_toast_snapshot
+ * get_toast_snapshot
*
- * Initialize an appropriate TOAST snapshot. We must use an MVCC snapshot
- * to initialize the TOAST snapshot; since we don't know which one to use,
- * just use the oldest one.
+ * Return the TOAST snapshot. Detoasting *must* happen in the same
+ * transaction that originally fetched the toast pointer.
*/
-void
-init_toast_snapshot(Snapshot toast_snapshot)
+Snapshot
+get_toast_snapshot(void)
{
- Snapshot snapshot = GetOldestSnapshot();
-
/*
- * GetOldestSnapshot returns NULL if the session has no active snapshots.
- * We can get that if, for example, a procedure fetches a toasted value
- * into a local variable, commits, and then tries to detoast the value.
- * Such coding is unsafe, because once we commit there is nothing to
- * prevent the toast data from being deleted. Detoasting *must* happen in
- * the same transaction that originally fetched the toast pointer. Hence,
- * rather than trying to band-aid over the problem, throw an error. (This
- * is not very much protection, because in many scenarios the procedure
- * would have already created a new transaction snapshot, preventing us
- * from detecting the problem. But it's better than nothing, and for sure
- * we shouldn't expend code on masking the problem more.)
+ * We cannot directly check that detoasting happens in the same
+ * transaction that originally fetched the toast pointer, but at least
+ * check that the session has some active snapshots. It might not if, for
+ * example, a procedure fetches a toasted value into a local variable,
+ * commits, and then tries to detoast the value. Such coding is unsafe,
+ * because once we commit there is nothing to prevent the toast data from
+ * being deleted. (This is not very much protection, because in many
+ * scenarios the procedure would have already created a new transaction
+ * snapshot, preventing us from detecting the problem. But it's better
+ * than nothing.)
*/
- if (snapshot == NULL)
+ if (!HaveRegisteredOrActiveSnapshot())
elog(ERROR, "cannot fetch toast data without an active snapshot");
- /*
- * Catalog snapshots can be returned by GetOldestSnapshot() even if not
- * registered or active. That easily hides bugs around not having a
- * snapshot set up - most of the time there is a valid catalog snapshot.
- * So additionally insist that the current snapshot is registered or
- * active.
- */
- Assert(HaveRegisteredOrActiveSnapshot());
-
- InitToastSnapshot(*toast_snapshot, snapshot->lsn, snapshot->whenTaken);
+ return &SnapshotToastData;
}
diff --git a/src/backend/access/heap/heaptoast.c b/src/backend/access/heap/heaptoast.c
index a420e16530..aae72bc2ab 100644
--- a/src/backend/access/heap/heaptoast.c
+++ b/src/backend/access/heap/heaptoast.c
@@ -639,7 +639,6 @@ heap_fetch_toast_slice(Relation toastrel, Oid valueid, int32 attrsize,
int endchunk;
int num_indexes;
int validIndex;
- SnapshotData SnapshotToast;
/* Look for the valid index of toast relation */
validIndex = toast_open_indexes(toastrel,
@@ -685,9 +684,8 @@ heap_fetch_toast_slice(Relation toastrel, Oid valueid, int32 attrsize,
}
/* Prepare for scan */
- init_toast_snapshot(&SnapshotToast);
toastscan = systable_beginscan_ordered(toastrel, toastidxs[validIndex],
- &SnapshotToast, nscankeys, toastkey);
+ get_toast_snapshot(), nscankeys, toastkey);
/*
* Read the chunks by index
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 36610a1c7e..c769b1aa3e 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -2135,8 +2135,6 @@ GetSnapshotDataReuse(Snapshot snapshot)
snapshot->active_count = 0;
snapshot->regd_count = 0;
snapshot->copied = false;
- snapshot->lsn = InvalidXLogRecPtr;
- snapshot->whenTaken = 0;
return true;
}
@@ -2516,8 +2514,6 @@ GetSnapshotData(Snapshot snapshot)
snapshot->active_count = 0;
snapshot->regd_count = 0;
snapshot->copied = false;
- snapshot->lsn = InvalidXLogRecPtr;
- snapshot->whenTaken = 0;
return snapshot;
}
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 7d2b34d4f2..b3fd6b102a 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -83,6 +83,7 @@ static SnapshotData SecondarySnapshotData = {SNAPSHOT_MVCC};
SnapshotData CatalogSnapshotData = {SNAPSHOT_MVCC};
SnapshotData SnapshotSelfData = {SNAPSHOT_SELF};
SnapshotData SnapshotAnyData = {SNAPSHOT_ANY};
+SnapshotData SnapshotToastData = {SNAPSHOT_TOAST};
/* Pointers to valid snapshots */
static Snapshot CurrentSnapshot = NULL;
@@ -119,9 +120,6 @@ typedef struct ActiveSnapshotElt
/* Top of the stack of active snapshots */
static ActiveSnapshotElt *ActiveSnapshot = NULL;
-/* Bottom of the stack of active snapshots */
-static ActiveSnapshotElt *OldestActiveSnapshot = NULL;
-
/*
* Currently registered Snapshots. Ordered in a heap by xmin, so that we can
* quickly find the one with lowest xmin, to advance our MyProc->xmin.
@@ -199,8 +197,6 @@ typedef struct SerializedSnapshotData
bool suboverflowed;
bool takenDuringRecovery;
CommandId curcid;
- TimestampTz whenTaken;
- XLogRecPtr lsn;
} SerializedSnapshotData;
/*
@@ -313,36 +309,6 @@ GetLatestSnapshot(void)
return SecondarySnapshot;
}
-/*
- * GetOldestSnapshot
- *
- * Get the transaction's oldest known snapshot, as judged by the LSN.
- * Will return NULL if there are no active or registered snapshots.
- */
-Snapshot
-GetOldestSnapshot(void)
-{
- Snapshot OldestRegisteredSnapshot = NULL;
- XLogRecPtr RegisteredLSN = InvalidXLogRecPtr;
-
- if (!pairingheap_is_empty(&RegisteredSnapshots))
- {
- OldestRegisteredSnapshot = pairingheap_container(SnapshotData, ph_node,
- pairingheap_first(&RegisteredSnapshots));
- RegisteredLSN = OldestRegisteredSnapshot->lsn;
- }
-
- if (OldestActiveSnapshot != NULL)
- {
- XLogRecPtr ActiveLSN = OldestActiveSnapshot->as_snap->lsn;
-
- if (XLogRecPtrIsInvalid(RegisteredLSN) || RegisteredLSN > ActiveLSN)
- return OldestActiveSnapshot->as_snap;
- }
-
- return OldestRegisteredSnapshot;
-}
-
/*
* GetCatalogSnapshot
* Get a snapshot that is sufficiently up-to-date for scan of the
@@ -684,8 +650,6 @@ PushActiveSnapshotWithLevel(Snapshot snapshot, int snap_level)
newactive->as_snap->active_count++;
ActiveSnapshot = newactive;
- if (OldestActiveSnapshot == NULL)
- OldestActiveSnapshot = ActiveSnapshot;
}
/*
@@ -756,8 +720,6 @@ PopActiveSnapshot(void)
pfree(ActiveSnapshot);
ActiveSnapshot = newstack;
- if (ActiveSnapshot == NULL)
- OldestActiveSnapshot = NULL;
SnapshotResetXmin();
}
@@ -980,8 +942,6 @@ AtSubAbort_Snapshot(int level)
pfree(ActiveSnapshot);
ActiveSnapshot = next;
- if (ActiveSnapshot == NULL)
- OldestActiveSnapshot = NULL;
}
SnapshotResetXmin();
@@ -1065,7 +1025,6 @@ AtEOXact_Snapshot(bool isCommit, bool resetXmin)
* it'll go away with TopTransactionContext.
*/
ActiveSnapshot = NULL;
- OldestActiveSnapshot = NULL;
pairingheap_reset(&RegisteredSnapshots);
CurrentSnapshot = NULL;
@@ -1727,8 +1686,6 @@ SerializeSnapshot(Snapshot snapshot, char *start_address)
serialized_snapshot.suboverflowed = snapshot->suboverflowed;
serialized_snapshot.takenDuringRecovery = snapshot->takenDuringRecovery;
serialized_snapshot.curcid = snapshot->curcid;
- serialized_snapshot.whenTaken = snapshot->whenTaken;
- serialized_snapshot.lsn = snapshot->lsn;
/*
* Ignore the SubXID array if it has overflowed, unless the snapshot was
@@ -1801,8 +1758,6 @@ RestoreSnapshot(char *start_address)
snapshot->suboverflowed = serialized_snapshot.suboverflowed;
snapshot->takenDuringRecovery = serialized_snapshot.takenDuringRecovery;
snapshot->curcid = serialized_snapshot.curcid;
- snapshot->whenTaken = serialized_snapshot.whenTaken;
- snapshot->lsn = serialized_snapshot.lsn;
snapshot->snapXactCompletionCount = 0;
/* Copy XIDs, if present. */
diff --git a/src/include/access/toast_internals.h b/src/include/access/toast_internals.h
index 0eeefe59fe..31a7e13c40 100644
--- a/src/include/access/toast_internals.h
+++ b/src/include/access/toast_internals.h
@@ -58,6 +58,6 @@ extern int toast_open_indexes(Relation toastrel,
int *num_indexes);
extern void toast_close_indexes(Relation *toastidxs, int num_indexes,
LOCKMODE lock);
-extern void init_toast_snapshot(Snapshot toast_snapshot);
+extern Snapshot get_toast_snapshot(void);
#endif /* TOAST_INTERNALS_H */
diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h
index 9398a84051..afc284e9c3 100644
--- a/src/include/utils/snapmgr.h
+++ b/src/include/utils/snapmgr.h
@@ -27,11 +27,14 @@ extern PGDLLIMPORT TransactionId RecentXmin;
/* Variables representing various special snapshot semantics */
extern PGDLLIMPORT SnapshotData SnapshotSelfData;
extern PGDLLIMPORT SnapshotData SnapshotAnyData;
+extern PGDLLIMPORT SnapshotData SnapshotToastData;
extern PGDLLIMPORT SnapshotData CatalogSnapshotData;
#define SnapshotSelf (&SnapshotSelfData)
#define SnapshotAny (&SnapshotAnyData)
+/* Use get_toast_snapshot() for the TOAST snapshot */
+
/*
* We don't provide a static SnapshotDirty variable because it would be
* non-reentrant. Instead, users of that snapshot type should declare a
@@ -49,15 +52,6 @@ extern PGDLLIMPORT SnapshotData CatalogSnapshotData;
((snapshotdata).snapshot_type = SNAPSHOT_NON_VACUUMABLE, \
(snapshotdata).vistest = (vistestp))
-/*
- * Similarly, some initialization is required for SnapshotToast. We need
- * to set lsn and whenTaken correctly to support snapshot_too_old.
- */
-#define InitToastSnapshot(snapshotdata, l, w) \
- ((snapshotdata).snapshot_type = SNAPSHOT_TOAST, \
- (snapshotdata).lsn = (l), \
- (snapshotdata).whenTaken = (w))
-
/* This macro encodes the knowledge of which snapshots are MVCC-safe */
#define IsMVCCSnapshot(snapshot) \
((snapshot)->snapshot_type == SNAPSHOT_MVCC || \
@@ -66,7 +60,6 @@ extern PGDLLIMPORT SnapshotData CatalogSnapshotData;
extern Snapshot GetTransactionSnapshot(void);
extern Snapshot GetLatestSnapshot(void);
extern void SnapshotSetCommandId(CommandId curcid);
-extern Snapshot GetOldestSnapshot(void);
extern Snapshot GetCatalogSnapshot(Oid relid);
extern Snapshot GetNonHistoricCatalogSnapshot(Oid relid);
diff --git a/src/include/utils/snapshot.h b/src/include/utils/snapshot.h
index 8d1e31e888..23306f3820 100644
--- a/src/include/utils/snapshot.h
+++ b/src/include/utils/snapshot.h
@@ -205,9 +205,6 @@ typedef struct SnapshotData
uint32 regd_count; /* refcount on RegisteredSnapshots */
pairingheap_node ph_node; /* link in the RegisteredSnapshots heap */
- TimestampTz whenTaken; /* timestamp when snapshot was taken */
- XLogRecPtr lsn; /* position in the WAL stream when taken */
-
/*
* The transaction completion count at the time GetSnapshotData() built
* this snapshot. Allows to avoid re-computing static snapshots when no
--
2.39.5
