From 7a32da753d05819c991d93cce3e3174f5a142238 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Wed, 4 Dec 2024 18:10:40 +0200 Subject: [PATCH 1/5] Remove unnecessary GetTransactionSnapshot() calls and FIXME comments In get_database_list() and get_subscription_list(), the GetTransactionSnapshot() call is not required because the catalog table scans use the catalog snapshot, which is held until the end of the scan. See table_beginscan_catalog(), which calls RegisterSnapshot(GetCatalogSnapshot(relid)). In InitPostgres, it's a little less obvious that it's not required, but still true I believe. All the catalog lookups in InitPostgres() also use the catalog snapshot, and looked up values are copied. Furthermore, as the removed comments said, calling GetTransactionSnapshot() didn't really prevent MyProc->xmin from being reset anyway. --- src/backend/postmaster/autovacuum.c | 11 +---------- src/backend/replication/logical/launcher.c | 11 +---------- src/backend/utils/init/postinit.c | 13 +------------ 3 files changed, 3 insertions(+), 32 deletions(-) diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index dc3cf87abab..8078eeef62e 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -1799,18 +1799,9 @@ get_database_list(void) resultcxt = CurrentMemoryContext; /* - * Start a transaction so we can access pg_database, and get a snapshot. - * We don't have a use for the snapshot itself, but we're interested in - * the secondary effect that it sets RecentGlobalXmin. (This is critical - * for anything that reads heap pages, because HOT may decide to prune - * them even if the process doesn't attempt to modify any tuples.) - * - * FIXME: This comment is inaccurate / the code buggy. A snapshot that is - * not pushed/active does not reliably prevent HOT pruning (->xmin could - * e.g. be cleared when cache invalidations are processed). + * Start a transaction so we can access pg_database. */ StartTransactionCommand(); - (void) GetTransactionSnapshot(); rel = table_open(DatabaseRelationId, AccessShareLock); scan = table_beginscan_catalog(rel, 0, NULL); diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c index e5fdca8bbf6..8b196420445 100644 --- a/src/backend/replication/logical/launcher.c +++ b/src/backend/replication/logical/launcher.c @@ -121,18 +121,9 @@ get_subscription_list(void) resultcxt = CurrentMemoryContext; /* - * Start a transaction so we can access pg_database, and get a snapshot. - * We don't have a use for the snapshot itself, but we're interested in - * the secondary effect that it sets RecentGlobalXmin. (This is critical - * for anything that reads heap pages, because HOT may decide to prune - * them even if the process doesn't attempt to modify any tuples.) - * - * FIXME: This comment is inaccurate / the code buggy. A snapshot that is - * not pushed/active does not reliably prevent HOT pruning (->xmin could - * e.g. be cleared when cache invalidations are processed). + * Start a transaction so we can access pg_subscription. */ StartTransactionCommand(); - (void) GetTransactionSnapshot(); rel = table_open(SubscriptionRelationId, AccessShareLock); scan = table_beginscan_catalog(rel, 0, NULL); diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 5b657a3f135..770ab6906e7 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -813,16 +813,7 @@ InitPostgres(const char *in_dbname, Oid dboid, } /* - * Start a new transaction here before first access to db, and get a - * snapshot. We don't have a use for the snapshot itself, but we're - * interested in the secondary effect that it sets RecentGlobalXmin. (This - * is critical for anything that reads heap pages, because HOT may decide - * to prune them even if the process doesn't attempt to modify any - * tuples.) - * - * FIXME: This comment is inaccurate / the code buggy. A snapshot that is - * not pushed/active does not reliably prevent HOT pruning (->xmin could - * e.g. be cleared when cache invalidations are processed). + * Start a new transaction here before first access to db. */ if (!bootstrap) { @@ -837,8 +828,6 @@ InitPostgres(const char *in_dbname, Oid dboid, * Fortunately, "read committed" is plenty good enough. */ XactIsoLevel = XACT_READ_COMMITTED; - - (void) GetTransactionSnapshot(); } /* -- 2.39.5