From: Heikki Linnakangas Date: Mon, 9 Dec 2024 16:13:03 +0000 (+0200) Subject: Remove remants of "snapshot too old" X-Git-Tag: REL_18_BETA1~1329 X-Git-Url: http://git.postgresql.org/gitweb/?a=commitdiff_plain;h=4d8275046c36792afb3604677c0a53c8530388ae;p=postgresql.git 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 reverts 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-6e6cd9b83f6d@iki.fi --- diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c index 9c74daaceed..e16557aca36 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 90d0654e629..1939cfb4d2a 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 a420e165304..aae72bc2abf 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 36610a1c7e7..c769b1aa3ef 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 7d2b34d4f20..a1a0c2adeb6 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(); } @@ -902,13 +864,6 @@ xmin_cmp(const pairingheap_node *a, const pairingheap_node *b, void *arg) * dropped. For efficiency, we only consider recomputing PGPROC->xmin when * the active snapshot stack is empty; this allows us not to need to track * which active snapshot is oldest. - * - * Note: it's tempting to use GetOldestSnapshot() here so that we can include - * active snapshots in the calculation. However, that compares by LSN not - * xmin so it's not entirely clear that it's the same thing. Also, we'd be - * critically dependent on the assumption that the bottommost active snapshot - * stack entry has the oldest xmin. (Current uses of GetOldestSnapshot() are - * not actually critical, but this would be.) */ static void SnapshotResetXmin(void) @@ -980,8 +935,6 @@ AtSubAbort_Snapshot(int level) pfree(ActiveSnapshot); ActiveSnapshot = next; - if (ActiveSnapshot == NULL) - OldestActiveSnapshot = NULL; } SnapshotResetXmin(); @@ -1065,7 +1018,6 @@ AtEOXact_Snapshot(bool isCommit, bool resetXmin) * it'll go away with TopTransactionContext. */ ActiveSnapshot = NULL; - OldestActiveSnapshot = NULL; pairingheap_reset(&RegisteredSnapshots); CurrentSnapshot = NULL; @@ -1727,8 +1679,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 +1751,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/genam.h b/src/include/access/genam.h index c25f5d11b53..81653febc18 100644 --- a/src/include/access/genam.h +++ b/src/include/access/genam.h @@ -14,9 +14,11 @@ #ifndef GENAM_H #define GENAM_H +#include "access/htup.h" #include "access/sdir.h" #include "access/skey.h" #include "nodes/tidbitmap.h" +#include "storage/buf.h" #include "storage/lockdefs.h" #include "utils/relcache.h" #include "utils/snapshot.h" diff --git a/src/include/access/toast_internals.h b/src/include/access/toast_internals.h index 0eeefe59fec..31a7e13c400 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/storage/predicate.h b/src/include/storage/predicate.h index d5c14986e24..5dca848cd8f 100644 --- a/src/include/storage/predicate.h +++ b/src/include/storage/predicate.h @@ -14,6 +14,7 @@ #ifndef PREDICATE_H #define PREDICATE_H +#include "storage/itemptr.h" #include "storage/lock.h" #include "utils/relcache.h" #include "utils/snapshot.h" diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h index 9398a84051c..afc284e9c36 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 8d1e31e888e..7d3ba38f2cf 100644 --- a/src/include/utils/snapshot.h +++ b/src/include/utils/snapshot.h @@ -13,11 +13,7 @@ #ifndef SNAPSHOT_H #define SNAPSHOT_H -#include "access/htup.h" -#include "access/xlogdefs.h" -#include "datatype/timestamp.h" #include "lib/pairingheap.h" -#include "storage/buf.h" /* @@ -205,9 +201,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