Assert in init_toast_snapshot() that some snapshot registered or active.

Commit <FIXME> fixed the bug that RemoveTempRelationsCallback() did not
push/register a snapshot. That only went unnoticed because often a valid
catalog snapshot exists and is returned by GetOldestSnapshot(). But due to
invalidation processing that is not reliable.

Thus assert in init_toast_snapshot() that there is a registered or active
snapshot, using the new HaveRegisteredOrActiveSnapshot().

Author: Andres Freund
Discussion: https://postgr.es/m/20220219180002.6tubjq7iw7m52bgd@alap3.anarazel.de

diff --git a/src/backend/access/common/toast_internals.c b/src/backend/access/common/toast_internals.c
index de37f561cade16f02bbef5f310bf394bba0bcac4..7052ac99780b4e3057f525f89e8840514c22262b 100644 (file)
--- a/src/backend/access/common/toast_internals.c
+++ b/src/backend/access/common/toast_internals.c
@@ -660,5 +660,14 @@ init_toast_snapshot(Snapshot toast_snapshot)
    if (snapshot == NULL)
        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);
 }

diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index a0b703a5195d6d326c5e518aea418ac1773e65d1..a0b81bf1549b5ae255f9407043adbb2c3be9e5d2 100644 (file)
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -1625,6 +1625,32 @@ ThereAreNoPriorRegisteredSnapshots(void)
    return false;
 }
 
+/*
+ * HaveRegisteredOrActiveSnapshots
+ *     Is there any registered or active snapshot?
+ *
+ * NB: Unless pushed or active, the cached catalog snapshot will not cause
+ * this function to return true. That allows this function to be used in
+ * checks enforcing a longer-lived snapshot.
+ */
+bool
+HaveRegisteredOrActiveSnapshot(void)
+{
+   if (ActiveSnapshot != NULL)
+       return true;
+
+   /*
+    * The catalog snapshot is in RegisteredSnapshots when valid, but can be
+    * removed at any time due to invalidation processing. If explicitly
+    * registered more than one snapshot has to be in RegisteredSnapshots.
+    */
+   if (pairingheap_is_empty(&RegisteredSnapshots) ||
+       !pairingheap_is_singular(&RegisteredSnapshots))
+       return false;
+
+   return CatalogSnapshot == NULL;
+}
+
 
 /*
  * Return a timestamp that is exactly on a minute boundary.

diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h
index 293c753034ae0d5b5bac237ea47a0604c58f9db3..e04018c034faf830181f142c4c9530863110babe 100644 (file)
--- a/src/include/utils/snapmgr.h
+++ b/src/include/utils/snapmgr.h
@@ -135,6 +135,7 @@ extern bool XactHasExportedSnapshots(void);
 extern void DeleteAllExportedSnapshotFiles(void);
 extern void WaitForOlderSnapshots(TransactionId limitXmin, bool progress);
 extern bool ThereAreNoPriorRegisteredSnapshots(void);
+extern bool HaveRegisteredOrActiveSnapshot(void);
 extern bool TransactionIdLimitedForOldSnapshots(TransactionId recentXmin,
                                                Relation relation,
                                                TransactionId *limit_xid,