Remove remants of "snapshot too old"
authorHeikki Linnakangas <[email protected]>
Mon, 9 Dec 2024 16:13:03 +0000 (18:13 +0200)
committerHeikki Linnakangas <[email protected]>
Mon, 9 Dec 2024 16:13:03 +0000 (18:13 +0200)
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

contrib/amcheck/verify_heapam.c
src/backend/access/common/toast_internals.c
src/backend/access/heap/heaptoast.c
src/backend/storage/ipc/procarray.c
src/backend/utils/time/snapmgr.c
src/include/access/genam.h
src/include/access/toast_internals.h
src/include/storage/predicate.h
src/include/utils/snapmgr.h
src/include/utils/snapshot.h

index 9c74daaceed1fb4814729516e24ae21c66080625..e16557aca36493326b73af0afe18d47353e12314 100644 (file)
@@ -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 =
index 90d0654e6298bb40910ccd2b5e59ba217d709a91..1939cfb4d2a93b068f3906998c8fa63646b92eaf 100644 (file)
@@ -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;
 }
index a420e1653046c2385ec6d8e679f5f15b7cb44d41..aae72bc2abf72af1dd7b146045c8cf9f8a6c6ea5 100644 (file)
@@ -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
index 36610a1c7e7f51faed76dca908e9e1f721eb5dac..c769b1aa3efbba8c9aba987e66b9695f456536ad 100644 (file)
@@ -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;
 }
index 7d2b34d4f20ea8ed1004ef59af1433fa0e18209e..a1a0c2adeb65c1e42418edb7ae470256e276f281 100644 (file)
@@ -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. */
index c25f5d11b53427706c1df2a259d4e2f7205c13f5..81653febc185dd028addd3bef20513ab777dbe7d 100644 (file)
 #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"
index 0eeefe59fec08641a927245616907aa918ac1594..31a7e13c4001fa52e8ef574d4f95e9ed6880b079 100644 (file)
@@ -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 */
index d5c14986e246ac27505c1f1612791f2d4abe593a..5dca848cd8fb3d8b32a2c49f4e94ffa5471262cf 100644 (file)
@@ -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"
index 9398a84051c5e864db1ea0032c83a6491f9801ab..afc284e9c3673c2c99ed91d0d28ae7ac6cc27756 100644 (file)
@@ -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);
index 8d1e31e888eea4d7a815d63e45f3d9c5be9174e3..7d3ba38f2cf444ee70fe3af3ac89eb2b1ecb911d 100644 (file)
 #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