Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 18 Jun 2026 10:03:40 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Sami Imseih <samimseih@gmail.com>
Cc: Ayush Tiwari <ayushtiwari.slg01@gmail.com>, zlh21343@163.com,
	pgsql-bugs@lists.postgresql.org, Andres Freund <andres@anarazel.de>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Subject: Re: BUG #19520: PANIC when concurrently manipulating stored
 procedures with pg_stat_statements and track_functions =
Message-ID: <ajND7FDG6MfiJfux@paquier.xyz>
References: <19520-73873648d44793cf@postgresql.org>
 <CAJTYsWVqtWF+KJUdoKrzUM9QPQA5qD225AoeBeN7cwT4V1qd6A@mail.gmail.com>
 <ajCJw956W3xsl8XP@paquier.xyz>
 <ajIfUavZvZqep3EQ@paquier.xyz>
 <CAJTYsWWJEchycUwkt6=Hk3j3ROOJKVp2n5yaU7JNqwqOmyoEMA@mail.gmail.com>
 <CAA5RZ0tuiNhkL30Juxwcox0C6pBCX6H-EVXZoRRQc4X6g6HTxQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="g0pPawYu3Zv2a1bV"
Content-Disposition: inline
In-Reply-To: 
 <CAA5RZ0tuiNhkL30Juxwcox0C6pBCX6H-EVXZoRRQc4X6g6HTxQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ajND7FDG6MfiJfux%40paquier.xyz>
Precedence: bulk


--g0pPawYu3Zv2a1bV
Content-Type: multipart/mixed; boundary="kbU6ROELxOap832x"
Content-Disposition: inline


--kbU6ROELxOap832x
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Jun 17, 2026 at 03:26:33PM -0500, Sami Imseih wrote:
>> Was the idea to leave it as a backstop, or would folding the handling in=
to
>> one place (or making _internal's an Assert) be cleaner?
>=20
> The check in _internal should be converted to an Assert. This documents
> that callers  must only pass "live" entries, which will be the case
> for all callers after
> the patch

Yeah, I was hesitating to do so, but perhaps you are right that there
is little meaning in keeping this extra elog() anymore in the internal
routine: all its callers discard entries marked as dropped.  And we do
so while holding an exclusive lock.

>> - In the missing_ok path the wrapper returns true, so the post-commit
>>   caller skips the not_freed_count++/GC request that a "real" not-freed
>>   drop would do.  That seems harmless since the entry self-heals
>>   but was returning true there a deliberate choice over mirroring
>>   the not-freed/false path? I need to take a look again at this, maybe
>>   I missed something.
>=20
> Finding an already dropped entry tells me that the first caller to drop t=
he
> entry also triggered a gc request, so we should not request it again.

Nope, we should not trigger multiple requests.

Attaching an updated patch for now.  I am still testing it locally
across all the branches to make sure that the issue is gone (that
takes quite a bit of time).  I'll probably apply it in a few hours
down to v15 if nothing pops up.
--
Michael

--kbU6ROELxOap832x
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename=v2-0001-Fix-potential-PANICs-with-concurrent-drop-of-pgst.patch
Content-Transfer-Encoding: quoted-printable

=46rom 1addb5f5e70ca6b4b30f79c6b7bd05fbf909bd6b Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Thu, 18 Jun 2026 09:57:32 +0900
Subject: [PATCH v2] Fix potential PANICs with concurrent drop of pgstats
 entries

---
 src/include/utils/pgstat_internal.h           |  3 ++-
 src/backend/utils/activity/pgstat.c           |  2 +-
 src/backend/utils/activity/pgstat_function.c  |  2 +-
 src/backend/utils/activity/pgstat_replslot.c  |  2 +-
 src/backend/utils/activity/pgstat_shmem.c     | 26 ++++++++++++-------
 src/backend/utils/activity/pgstat_xact.c      |  8 +++---
 .../test_custom_stats/test_custom_var_stats.c |  2 +-
 7 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat=
_internal.h
index fe463faaf639..3ca4f4548956 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -807,7 +807,8 @@ extern PgStat_EntryRef *pgstat_get_entry_ref(PgStat_Kin=
d kind, Oid dboid, uint64
 extern bool pgstat_lock_entry(PgStat_EntryRef *entry_ref, bool nowait);
 extern bool pgstat_lock_entry_shared(PgStat_EntryRef *entry_ref, bool nowa=
it);
 extern void pgstat_unlock_entry(PgStat_EntryRef *entry_ref);
-extern bool pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint64 objid);
+extern bool pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint64 objid,
+							  bool missing_ok);
 extern void pgstat_drop_all_entries(void);
 extern void pgstat_drop_matching_entries(bool (*do_drop) (PgStatShared_Has=
hEntry *, Datum),
 										 Datum match_data);
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activi=
ty/pgstat.c
index b67da88c7dc2..c4fa14f138fa 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -650,7 +650,7 @@ pgstat_shutdown_hook(int code, Datum arg)
 	dlist_init(&pgStatPending);
=20
 	/* drop the backend stats entry */
-	if (!pgstat_drop_entry(PGSTAT_KIND_BACKEND, InvalidOid, MyProcNumber))
+	if (!pgstat_drop_entry(PGSTAT_KIND_BACKEND, InvalidOid, MyProcNumber, fal=
se))
 		pgstat_request_entry_refs_gc();
=20
 	pgstat_detach_shmem();
diff --git a/src/backend/utils/activity/pgstat_function.c b/src/backend/uti=
ls/activity/pgstat_function.c
index d47d05e3d922..f0366e139907 100644
--- a/src/backend/utils/activity/pgstat_function.c
+++ b/src/backend/utils/activity/pgstat_function.c
@@ -113,7 +113,7 @@ pgstat_init_function_usage(FunctionCallInfo fcinfo,
 		if (!SearchSysCacheExists1(PROCOID, ObjectIdGetDatum(fcinfo->flinfo->fn_=
oid)))
 		{
 			pgstat_drop_entry(PGSTAT_KIND_FUNCTION, MyDatabaseId,
-							  fcinfo->flinfo->fn_oid);
+							  fcinfo->flinfo->fn_oid, true);
 			ereport(ERROR, errcode(ERRCODE_UNDEFINED_FUNCTION),
 					errmsg("function call to dropped function"));
 		}
diff --git a/src/backend/utils/activity/pgstat_replslot.c b/src/backend/uti=
ls/activity/pgstat_replslot.c
index 0d00dd5d93aa..a32b70a03736 100644
--- a/src/backend/utils/activity/pgstat_replslot.c
+++ b/src/backend/utils/activity/pgstat_replslot.c
@@ -188,7 +188,7 @@ pgstat_drop_replslot(ReplicationSlot *slot)
 	Assert(LWLockHeldByMeInMode(ReplicationSlotAllocationLock, LW_EXCLUSIVE));
=20
 	if (!pgstat_drop_entry(PGSTAT_KIND_REPLSLOT, InvalidOid,
-						   ReplicationSlotIndex(slot)))
+						   ReplicationSlotIndex(slot), false))
 		pgstat_request_entry_refs_gc();
 }
=20
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/=
activity/pgstat_shmem.c
index b8f354c818a0..47c8f927e5b4 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -917,14 +917,7 @@ pgstat_drop_entry_internal(PgStatShared_HashEntry *she=
nt,
 	 * Signal that the entry is dropped - this will eventually cause other
 	 * backends to release their references.
 	 */
-	if (shent->dropped)
-		elog(ERROR,
-			 "trying to drop stats entry already dropped: kind=3D%s dboid=3D%u obji=
d=3D%" PRIu64 " refcount=3D%u generation=3D%u",
-			 pgstat_get_kind_info(shent->key.kind)->name,
-			 shent->key.dboid,
-			 shent->key.objid,
-			 pg_atomic_read_u32(&shent->refcount),
-			 pg_atomic_read_u32(&shent->generation));
+	Assert(!shent->dropped);
 	shent->dropped =3D true;
=20
 	/* release refcount marking entry as not dropped */
@@ -1000,13 +993,16 @@ pgstat_drop_database_and_contents(Oid dboid)
  * This routine returns false if the stats entry of the dropped object cou=
ld
  * not be freed, true otherwise.
  *
+ * If missing_ok is true, skip entries that have been concurrently dropped.
+ *
  * The callers of this function should call pgstat_request_entry_refs_gc()
  * if the stats entry could not be freed, to ensure that this entry's memo=
ry
  * can be reclaimed later by a different backend calling
  * pgstat_gc_entry_refs().
  */
 bool
-pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint64 objid)
+pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint64 objid,
+				  bool missing_ok)
 {
 	PgStat_HashKey key =3D {0};
 	PgStatShared_HashEntry *shent;
@@ -1031,6 +1027,18 @@ pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint6=
4 objid)
 	shent =3D dshash_find(pgStatLocal.shared_hash, &key, true);
 	if (shent)
 	{
+		if (shent->dropped)
+		{
+			if (!missing_ok)
+				elog(ERROR,
+					 "trying to drop stats entry already dropped: kind=3D%s dboid=3D%u ob=
jid=3D%" PRIu64,
+					 pgstat_get_kind_info(shent->key.kind)->name,
+					 shent->key.dboid,
+					 shent->key.objid);
+			dshash_release_lock(pgStatLocal.shared_hash, shent);
+			return true;
+		}
+
 		freed =3D pgstat_drop_entry_internal(shent, NULL);
=20
 		/*
diff --git a/src/backend/utils/activity/pgstat_xact.c b/src/backend/utils/a=
ctivity/pgstat_xact.c
index 5e2d69e62979..3e1978775e16 100644
--- a/src/backend/utils/activity/pgstat_xact.c
+++ b/src/backend/utils/activity/pgstat_xact.c
@@ -85,7 +85,7 @@ AtEOXact_PgStat_DroppedStats(PgStat_SubXactStatus *xact_s=
tate, bool isCommit)
 			 * Transaction that dropped an object committed. Drop the stats
 			 * too.
 			 */
-			if (!pgstat_drop_entry(it->kind, it->dboid, objid))
+			if (!pgstat_drop_entry(it->kind, it->dboid, objid, true))
 				not_freed_count++;
 		}
 		else if (!isCommit && pending->is_create)
@@ -94,7 +94,7 @@ AtEOXact_PgStat_DroppedStats(PgStat_SubXactStatus *xact_s=
tate, bool isCommit)
 			 * Transaction that created an object aborted. Drop the stats
 			 * associated with the object.
 			 */
-			if (!pgstat_drop_entry(it->kind, it->dboid, objid))
+			if (!pgstat_drop_entry(it->kind, it->dboid, objid, true))
 				not_freed_count++;
 		}
=20
@@ -160,7 +160,7 @@ AtEOSubXact_PgStat_DroppedStats(PgStat_SubXactStatus *x=
act_state,
 			 * Subtransaction creating a new stats object aborted. Drop the
 			 * stats object.
 			 */
-			if (!pgstat_drop_entry(it->kind, it->dboid, objid))
+			if (!pgstat_drop_entry(it->kind, it->dboid, objid, true))
 				not_freed_count++;
 			pfree(pending);
 		}
@@ -323,7 +323,7 @@ pgstat_execute_transactional_drops(int ndrops, struct x=
l_xact_stats_item *items,
 		xl_xact_stats_item *it =3D &items[i];
 		uint64		objid =3D ((uint64) it->objid_hi) << 32 | it->objid_lo;
=20
-		if (!pgstat_drop_entry(it->kind, it->dboid, objid))
+		if (!pgstat_drop_entry(it->kind, it->dboid, objid, true))
 			not_freed_count++;
 	}
=20
diff --git a/src/test/modules/test_custom_stats/test_custom_var_stats.c b/s=
rc/test/modules/test_custom_stats/test_custom_var_stats.c
index 5c4871ed37ca..863d6a524925 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
@@ -600,7 +600,7 @@ test_custom_stats_var_drop(PG_FUNCTION_ARGS)
=20
 	/* Drop entry and request GC if the entry could not be freed */
 	if (!pgstat_drop_entry(PGSTAT_KIND_TEST_CUSTOM_VAR_STATS, InvalidOid,
-						   PGSTAT_CUSTOM_VAR_STATS_IDX(stat_name)))
+						   PGSTAT_CUSTOM_VAR_STATS_IDX(stat_name), false))
 		pgstat_request_entry_refs_gc();
=20
 	PG_RETURN_VOID();
--=20
2.54.0


--kbU6ROELxOap832x--

--g0pPawYu3Zv2a1bV
Content-Type: application/pgp-signature; name=signature.asc

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmozQ+wACgkQnvQgOdby
QH1LYQ/9HWjK+Tjv9LrW8wWYZVoNZOwDaHvczfgD6V8CAwU4bsMf7SEiGIcLlZzM
buybDHwQRon2s9Jiavu0iK/ug2X4ZHAebmctSL+GnvLY4NeCIYfR7grGrP9WAIG/
R4LXg5FTb43JyH2HT4nBWbE+ZsdIT6ZWXImQKzJ7IKchcc5xLOekUohhEzeqwPN3
M1hH1RYQ4iStPrDTuazh4pG90dqnXTxUSjl4VBco1SW0L/WY7zC8GLRzAc6KEhdu
hcDrc92/AOUhvyrmZjlqDnWGWe4Al4rhvSk8Y+Mij+7PWpdmkDp3RBJHnEUclfIs
dr44DWz5NC+OUd/VKSyZdWH0cfKMIjWwfcvlGBfOz9QBqnQHb7I6B+AD03yvRINP
ao7frDaVf1OIT5PWklvEl9/Unx3nV5+sglVej4XR3Lcyt2HKn/n69XeoATXR8dj9
qrXSvAuJ5rng9q/zjlAAwS+BmdbkNjZjdyZyXKiQRgZPRx83cwIUJ8IGqr1v3BEW
Qrl/kULyn3CjFVCAtubm5aOU68/Y9Je1dAUxfltPzY/JKWhNsnR2Pe/H6oxKQDXB
yZ1cQepn52SSP0foZaZEkSmkh6o2nsYkNFtmch4tLrkpeI4pjwnKCU1z7GDBAmfK
AMvxsWOpJm8/OtN1DE6fo6SX1qZtXgozoTP1HDduVJgFOVOvgPM=
=2mJg
-----END PGP SIGNATURE-----

--g0pPawYu3Zv2a1bV--