Date: Thu, 9 Apr 2026 11:34:19 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Sami Imseih <samimseih@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>,
	Alexander Lakhin <exclusion@gmail.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Robert Treat <rob@xzilla.net>, satyanarlapuram@gmail.com,
	pgsql-hackers <pgsql-hackers@postgresql.org>, tndrwang@gmail.com
Subject: Re: Add pg_stat_autovacuum_priority
Message-ID: <adfVC4hWD28tcCpT@nathan>
References: <adaWuTR7oCKodH7k@nathan>
 <3097984.1775674400@sss.pgh.pa.us>
 <adan_E0o69p81lIj@nathan>
 <3101163.1775676098@sss.pgh.pa.us>
 <u3akkfw3vaocviudt6f7ft4kxjc2273rh3tfhz5rfwg6jrcc2t@wurwws6wgjjz>
 <adbHYQMZ0pHQttWa@nathan>
 <CAA5RZ0uL1ukKvV0Gwgok9ut_203q_E_tzBrwhDAuJCKVNwWZVw@mail.gmail.com>
 <adbNL1puSbBi8nMo@nathan>
 <CAA5RZ0uiqzLPvZMXsooFPR5udpXUiVy8WvTLbduOvT2JPawYkA@mail.gmail.com>
 <adb6vVe4-JVq8RgU@nathan>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="KrZsvcbSG1UCPWv8"
Content-Disposition: inline
In-Reply-To: <adb6vVe4-JVq8RgU@nathan>
Archived-At: <https://www.postgresql.org/message-id/adfVC4hWD28tcCpT%40nathan>
Precedence: bulk


--KrZsvcbSG1UCPWv8
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Here's what I have staged for commit.

-- 
nathan

--KrZsvcbSG1UCPWv8
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename=v3-0001-Fix-double-free-in-pg_stat_autovacuum_scores.patch

From 69e545eb08a14c00ac361bfc4a2db85cd56a8226 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 9 Apr 2026 11:20:21 -0500
Subject: [PATCH v3 1/1] Fix double-free in pg_stat_autovacuum_scores.

Presently, relation_needs_vacanalyze() unconditionally frees the
pgstat entry returned by pgstat_fetch_stat_tabentry_ext().  This
behavior was first added by commit 02502c1bca to avoid memory
leakage in autovacuum.  While this is fine for autovacuum since it
forces stats_fetch_consistency to "none", it is not okay for other
callers that use "cache" or "snapshot".  This manifests as a
double-free when pg_stat_autovacuum_scores is called multiple times
in the same transaction.

To fix, add a "bool *may_free" parameter to
pgstat_fetch_stat_tabentry_ext() that returns whether it is safe
for the caller to explicitly pfree() the result if desired.  If a
caller would rather leave it to the memory context machinery to
free the result, it can pass NULL for the "may_free" argument (or
just ignore its value).

Oversight in commit 87f61f0c82.

Reported-by: Tender Wang <tndrwang@gmail.com>
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Suggested-by: Andres Freund <andres@anarazel.de>
Suggested-by: Tom Lane <tgl@sss.pgh.pa.us>
Author: Sami Imseih <samimseih@gmail.com>
Discussion: https://postgr.es/m/CAHewXNkJKdwb3D5OnksrdOqzqUnXUEMpDam1TPW0vfUkW%3D7jUw%40mail.gmail.com
Discussion: https://postgr.es/m/5684f479-858e-4c5d-b8f5-bcf05de1f909%40gmail.com
---
 src/backend/postmaster/autovacuum.c            |  7 +++++--
 src/backend/utils/activity/pgstat.c            | 18 +++++++++++++++++-
 src/backend/utils/activity/pgstat_backend.c    |  3 ++-
 src/backend/utils/activity/pgstat_database.c   |  2 +-
 src/backend/utils/activity/pgstat_function.c   |  2 +-
 src/backend/utils/activity/pgstat_relation.c   | 12 +++++++-----
 src/backend/utils/activity/pgstat_replslot.c   |  3 ++-
 .../utils/activity/pgstat_subscription.c       |  2 +-
 src/include/pgstat.h                           |  3 ++-
 src/include/utils/pgstat_internal.h            |  3 ++-
 .../test_custom_stats/test_custom_var_stats.c  |  3 ++-
 11 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index f9fa874b79f..82061247988 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -3080,6 +3080,7 @@ relation_needs_vacanalyze(Oid relid,
 	PgStat_StatTabEntry *tabentry;
 	bool		force_vacuum;
 	bool		av_enabled;
+	bool		may_free = false;
 
 	/* constants from reloptions or GUC variables */
 	int			vac_base_thresh,
@@ -3246,7 +3247,7 @@ relation_needs_vacanalyze(Oid relid,
 	 * forced.
 	 */
 	tabentry = pgstat_fetch_stat_tabentry_ext(classForm->relisshared,
-											  relid);
+											  relid, &may_free);
 	if (!tabentry)
 		return;
 
@@ -3327,7 +3328,9 @@ relation_needs_vacanalyze(Oid relid,
 			 anltuples, anlthresh, scores->anl,
 			 scores->xid, scores->mxid);
 
-	pfree(tabentry);
+	/* Avoid leaking pgstat entries until the end of autovacuum. */
+	if (may_free)
+		pfree(tabentry);
 }
 
 /*
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index eb8ccbaa628..a59fb7c66b6 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -960,7 +960,7 @@ pgstat_clear_snapshot(void)
 }
 
 void *
-pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, uint64 objid)
+pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, uint64 objid, bool *may_free)
 {
 	PgStat_HashKey key = {0};
 	PgStat_EntryRef *entry_ref;
@@ -971,6 +971,13 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, uint64 objid)
 	Assert(IsUnderPostmaster || !IsPostmasterEnvironment);
 	Assert(!kind_info->fixed_amount);
 
+	/*
+	 * Initialize *may_free to false.  We'll change it to true later if we end
+	 * up allocating the result in the caller's context and not caching it.
+	 */
+	if (may_free)
+		*may_free = false;
+
 	pgstat_prep_snapshot();
 
 	key.kind = kind;
@@ -1024,7 +1031,16 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, uint64 objid)
 	 * repeated accesses.
 	 */
 	if (pgstat_fetch_consistency == PGSTAT_FETCH_CONSISTENCY_NONE)
+	{
 		stats_data = palloc(kind_info->shared_data_len);
+
+		/*
+		 * Since we allocated in the caller's context and aren't caching the
+		 * result, it is safe to pfree() the result.
+		 */
+		if (may_free)
+			*may_free = true;
+	}
 	else
 		stats_data = MemoryContextAlloc(pgStatLocal.snapshot.context,
 										kind_info->shared_data_len);
diff --git a/src/backend/utils/activity/pgstat_backend.c b/src/backend/utils/activity/pgstat_backend.c
index 04fe13e64c6..73461c9bca5 100644
--- a/src/backend/utils/activity/pgstat_backend.c
+++ b/src/backend/utils/activity/pgstat_backend.c
@@ -95,7 +95,8 @@ pgstat_fetch_stat_backend(ProcNumber procNumber)
 	PgStat_Backend *backend_entry;
 
 	backend_entry = (PgStat_Backend *) pgstat_fetch_entry(PGSTAT_KIND_BACKEND,
-														  InvalidOid, procNumber);
+														  InvalidOid, procNumber,
+														  NULL);
 
 	return backend_entry;
 }
diff --git a/src/backend/utils/activity/pgstat_database.c b/src/backend/utils/activity/pgstat_database.c
index 933dcb5cae5..f1846d3236c 100644
--- a/src/backend/utils/activity/pgstat_database.c
+++ b/src/backend/utils/activity/pgstat_database.c
@@ -288,7 +288,7 @@ PgStat_StatDBEntry *
 pgstat_fetch_stat_dbentry(Oid dboid)
 {
 	return (PgStat_StatDBEntry *)
-		pgstat_fetch_entry(PGSTAT_KIND_DATABASE, dboid, InvalidOid);
+		pgstat_fetch_entry(PGSTAT_KIND_DATABASE, dboid, InvalidOid, NULL);
 }
 
 void
diff --git a/src/backend/utils/activity/pgstat_function.c b/src/backend/utils/activity/pgstat_function.c
index e6b84283c6c..d47d05e3d92 100644
--- a/src/backend/utils/activity/pgstat_function.c
+++ b/src/backend/utils/activity/pgstat_function.c
@@ -245,5 +245,5 @@ PgStat_StatFuncEntry *
 pgstat_fetch_stat_funcentry(Oid func_id)
 {
 	return (PgStat_StatFuncEntry *)
-		pgstat_fetch_entry(PGSTAT_KIND_FUNCTION, MyDatabaseId, func_id);
+		pgstat_fetch_entry(PGSTAT_KIND_FUNCTION, MyDatabaseId, func_id, NULL);
 }
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index bc8c43b96aa..b2ca28f83ba 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -61,7 +61,8 @@ pgstat_copy_relation_stats(Relation dst, Relation src)
 	PgStat_EntryRef *dst_ref;
 
 	srcstats = pgstat_fetch_stat_tabentry_ext(src->rd_rel->relisshared,
-											  RelationGetRelid(src));
+											  RelationGetRelid(src),
+											  NULL);
 	if (!srcstats)
 		return;
 
@@ -468,20 +469,21 @@ pgstat_update_heap_dead_tuples(Relation rel, int delta)
 PgStat_StatTabEntry *
 pgstat_fetch_stat_tabentry(Oid relid)
 {
-	return pgstat_fetch_stat_tabentry_ext(IsSharedRelation(relid), relid);
+	return pgstat_fetch_stat_tabentry_ext(IsSharedRelation(relid), relid, NULL);
 }
 
 /*
  * More efficient version of pgstat_fetch_stat_tabentry(), allowing to specify
- * whether the to-be-accessed table is a shared relation or not.
+ * whether the to-be-accessed table is a shared relation or not.  This version
+ * also returns whether the caller can pfree() the result if desired.
  */
 PgStat_StatTabEntry *
-pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid)
+pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid, bool *may_free)
 {
 	Oid			dboid = (shared ? InvalidOid : MyDatabaseId);
 
 	return (PgStat_StatTabEntry *)
-		pgstat_fetch_entry(PGSTAT_KIND_RELATION, dboid, reloid);
+		pgstat_fetch_entry(PGSTAT_KIND_RELATION, dboid, reloid, may_free);
 }
 
 /*
diff --git a/src/backend/utils/activity/pgstat_replslot.c b/src/backend/utils/activity/pgstat_replslot.c
index 168ef8f8f45..0d00dd5d93a 100644
--- a/src/backend/utils/activity/pgstat_replslot.c
+++ b/src/backend/utils/activity/pgstat_replslot.c
@@ -208,7 +208,8 @@ pgstat_fetch_replslot(NameData slotname)
 
 	if (idx != -1)
 		slotentry = (PgStat_StatReplSlotEntry *) pgstat_fetch_entry(PGSTAT_KIND_REPLSLOT,
-																	InvalidOid, idx);
+																	InvalidOid, idx,
+																	NULL);
 
 	LWLockRelease(ReplicationSlotControlLock);
 
diff --git a/src/backend/utils/activity/pgstat_subscription.c b/src/backend/utils/activity/pgstat_subscription.c
index 3277cf88a4e..3eaf3e0390f 100644
--- a/src/backend/utils/activity/pgstat_subscription.c
+++ b/src/backend/utils/activity/pgstat_subscription.c
@@ -107,7 +107,7 @@ PgStat_StatSubEntry *
 pgstat_fetch_stat_subscription(Oid subid)
 {
 	return (PgStat_StatSubEntry *)
-		pgstat_fetch_entry(PGSTAT_KIND_SUBSCRIPTION, InvalidOid, subid);
+		pgstat_fetch_entry(PGSTAT_KIND_SUBSCRIPTION, InvalidOid, subid, NULL);
 }
 
 /*
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 2786a7c5ffb..dfa2e837638 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -763,7 +763,8 @@ extern void pgstat_twophase_postabort(FullTransactionId fxid, uint16 info,
 
 extern PgStat_StatTabEntry *pgstat_fetch_stat_tabentry(Oid relid);
 extern PgStat_StatTabEntry *pgstat_fetch_stat_tabentry_ext(bool shared,
-														   Oid reloid);
+														   Oid reloid,
+														   bool *may_free);
 extern PgStat_TableStatus *find_tabstat_entry(Oid rel_id);
 
 
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index c0496f2c69c..fe463faaf63 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -685,7 +685,8 @@ extern PgStat_EntryRef *pgstat_prep_pending_entry(PgStat_Kind kind, Oid dboid,
 extern PgStat_EntryRef *pgstat_fetch_pending_entry(PgStat_Kind kind,
 												   Oid dboid, uint64 objid);
 
-extern void *pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, uint64 objid);
+extern void *pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, uint64 objid,
+								bool *may_free);
 extern void pgstat_snapshot_fixed(PgStat_Kind kind);
 
 
diff --git a/src/test/modules/test_custom_stats/test_custom_var_stats.c b/src/test/modules/test_custom_stats/test_custom_var_stats.c
index 2ef0e903745..5c4871ed37c 100644
--- a/src/test/modules/test_custom_stats/test_custom_var_stats.c
+++ b/src/test/modules/test_custom_stats/test_custom_var_stats.c
@@ -494,7 +494,8 @@ test_custom_stats_var_fetch_entry(const char *stat_name)
 	return (PgStat_StatCustomVarEntry *)
 		pgstat_fetch_entry(PGSTAT_KIND_TEST_CUSTOM_VAR_STATS,
 						   InvalidOid,
-						   PGSTAT_CUSTOM_VAR_STATS_IDX(stat_name));
+						   PGSTAT_CUSTOM_VAR_STATS_IDX(stat_name),
+						   NULL);
 }
 
 /*--------------------------------------------------------------------------
-- 
2.50.1 (Apple Git-155)


--KrZsvcbSG1UCPWv8--