shihao zhong <[email protected]> writes:
>> Do we really need a serializable snapshot for reading the notify
>> queue? If we do, can we acquire it before making any important state
>> changes?
> I did some investigation today and I don't think we need a serializable
> snapshot.
I looked at this, and I agree: all we care about in async.c is which
transactions are still running, and there's no particular need to wait
for a safe snapshot before deciding that. (A different way to say
that, perhaps, is that NOTIFY messages are delivered on the same basis
as READ COMMITTED operations, even if your default transaction mode
is higher.)
> So we just need to call GetSnapshotData(), but that needs a snapshot.
> I created a static one in async.c because feels like we should not
> touch snapmgr for encapsulation. If you'd rather keep this in
> snapmgr.c, a small wrapper there would work too.
I don't like that choice at all. The only callers of
GetSnapshotData() are in snapmgr.c and predicate.c, and there are
enough weird optimization-driven assumptions around snapshot
management that I think any new caller had better be in one of those
modules so that people working in that area will notice it. I wrote
a wrapper that documents two of the assumptions I think are involved
here, and there could well be more. See what you think.
regards, tom lane
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 539bc9cb0e3..a9777a7e838 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -2656,9 +2656,14 @@ asyncQueueReadAllNotifications(void)
* BecomeRegisteredListener has already added us to the listener array,
* so no not-yet-committed messages can be removed from the queue
* before we see them.
+ *
+ * Since we'll only use this snapshot to check whether XIDs are still
+ * running, and we will release it before returning, it's okay to use
+ * GetSecondarySnapshot(); that avoids possibly blocking on other
+ * transactions in SERIALIZABLE READ ONLY DEFERRABLE mode.
*----------
*/
- snapshot = RegisterSnapshot(GetLatestSnapshot());
+ snapshot = RegisterSnapshot(GetSecondarySnapshot());
/*
* It is possible that we fail while trying to send a message to our
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 048450da2d3..6defd7203b1 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -376,6 +376,36 @@ GetLatestSnapshot(void)
return SecondarySnapshot;
}
+/*
+ * GetSecondarySnapshot
+ * Get a snapshot that is up-to-date as of the current instant.
+ *
+ * This differs from GetLatestSnapshot() in that it will never set the
+ * transaction snapshot, so that it does not need to block waiting for a safe
+ * snapshot in RO deferrable serializable transaction mode. That makes it
+ * semantically dubious to use the result for testing tuple visibility.
+ * Current callers only use it to test whether XIDs are still running.
+ *
+ * An additional constraint is that if the caller registers the snapshot, it
+ * must unregister it before any possible call of GetTransactionSnapshot();
+ * else the empty-heap Assert therein could fire.
+ */
+Snapshot
+GetSecondarySnapshot(void)
+{
+ /*
+ * We might be able to relax this, but nothing that could otherwise work
+ * needs it.
+ */
+ if (IsInParallelMode())
+ elog(ERROR,
+ "cannot update SecondarySnapshot during a parallel operation");
+
+ SecondarySnapshot = GetSnapshotData(&SecondarySnapshotData);
+
+ return SecondarySnapshot;
+}
+
/*
* GetCatalogSnapshot
* Get a snapshot that is sufficiently up-to-date for scan of the
diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h
index 1c550096393..be3b2e5e5c9 100644
--- a/src/include/utils/snapmgr.h
+++ b/src/include/utils/snapmgr.h
@@ -76,6 +76,7 @@ extern PGDLLIMPORT SnapshotData SnapshotToastData;
extern Snapshot GetTransactionSnapshot(void);
extern Snapshot GetLatestSnapshot(void);
+extern Snapshot GetSecondarySnapshot(void);
extern void SnapshotSetCommandId(CommandId curcid);
extern Snapshot GetCatalogSnapshot(Oid relid);
