From 19b6beddc3385a465623a3afef6595f0e4c4235f Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Sat, 19 Feb 2022 12:42:37 -0800 Subject: [PATCH v1 2/2] Assert in init_toast_snapshot() that some snapshot registered or active. Commit 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 --- src/include/utils/snapmgr.h | 1 + src/backend/access/common/toast_internals.c | 9 +++++++ src/backend/utils/time/snapmgr.c | 26 +++++++++++++++++++++ 3 files changed, 36 insertions(+) diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h index 293c753034a..e04018c034f 100644 --- 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, diff --git a/src/backend/access/common/toast_internals.c b/src/backend/access/common/toast_internals.c index de37f561cad..7052ac99780 100644 --- 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 a0b703a5195..a0b81bf1549 100644 --- 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. -- 2.34.0