public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Sami Imseih <[email protected]>
Cc: Ayush Tiwari <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Andres Freund <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Subject: Re: BUG #19520: PANIC when concurrently manipulating stored procedures with pg_stat_statements and track_functions =
Date: Thu, 18 Jun 2026 10:03:40 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAA5RZ0tuiNhkL30Juxwcox0C6pBCX6H-EVXZoRRQc4X6g6HTxQ@mail.gmail.com>
References: <[email protected]>
<CAJTYsWVqtWF+KJUdoKrzUM9QPQA5qD225AoeBeN7cwT4V1qd6A@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAJTYsWWJEchycUwkt6=Hk3j3ROOJKVp2n5yaU7JNqwqOmyoEMA@mail.gmail.com>
<CAA5RZ0tuiNhkL30Juxwcox0C6pBCX6H-EVXZoRRQc4X6g6HTxQ@mail.gmail.com>
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 into
>> one place (or making _internal's an Assert) be cleaner?
>
> 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.
>
> Finding an already dropped entry tells me that the first caller to drop the
> 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
From 1addb5f5e70ca6b4b30f79c6b7bd05fbf909bd6b Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
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_Kind 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 nowait);
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_HashEntry *, Datum),
Datum match_data);
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/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);
/* drop the backend stats entry */
- if (!pgstat_drop_entry(PGSTAT_KIND_BACKEND, InvalidOid, MyProcNumber))
+ if (!pgstat_drop_entry(PGSTAT_KIND_BACKEND, InvalidOid, MyProcNumber, false))
pgstat_request_entry_refs_gc();
pgstat_detach_shmem();
diff --git a/src/backend/utils/activity/pgstat_function.c b/src/backend/utils/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/utils/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));
if (!pgstat_drop_entry(PGSTAT_KIND_REPLSLOT, InvalidOid,
- ReplicationSlotIndex(slot)))
+ ReplicationSlotIndex(slot), false))
pgstat_request_entry_refs_gc();
}
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 *shent,
* 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=%s dboid=%u objid=%" PRIu64 " refcount=%u generation=%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 = true;
/* 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 could
* 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 memory
* 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 = {0};
PgStatShared_HashEntry *shent;
@@ -1031,6 +1027,18 @@ pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint64 objid)
shent = 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=%s dboid=%u objid=%" 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 = pgstat_drop_entry_internal(shent, NULL);
/*
diff --git a/src/backend/utils/activity/pgstat_xact.c b/src/backend/utils/activity/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_state, 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_state, 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++;
}
@@ -160,7 +160,7 @@ AtEOSubXact_PgStat_DroppedStats(PgStat_SubXactStatus *xact_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 xl_xact_stats_item *items,
xl_xact_stats_item *it = &items[i];
uint64 objid = ((uint64) it->objid_hi) << 32 | it->objid_lo;
- if (!pgstat_drop_entry(it->kind, it->dboid, objid))
+ if (!pgstat_drop_entry(it->kind, it->dboid, objid, true))
not_freed_count++;
}
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 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)
/* 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();
PG_RETURN_VOID();
--
2.54.0
Attachments:
[text/plain] v2-0001-Fix-potential-PANICs-with-concurrent-drop-of-pgst.patch (7.7K, ../[email protected]/2-v2-0001-Fix-potential-PANICs-with-concurrent-drop-of-pgst.patch)
download | inline diff:
From 1addb5f5e70ca6b4b30f79c6b7bd05fbf909bd6b Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
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_Kind 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 nowait);
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_HashEntry *, Datum),
Datum match_data);
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/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);
/* drop the backend stats entry */
- if (!pgstat_drop_entry(PGSTAT_KIND_BACKEND, InvalidOid, MyProcNumber))
+ if (!pgstat_drop_entry(PGSTAT_KIND_BACKEND, InvalidOid, MyProcNumber, false))
pgstat_request_entry_refs_gc();
pgstat_detach_shmem();
diff --git a/src/backend/utils/activity/pgstat_function.c b/src/backend/utils/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/utils/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));
if (!pgstat_drop_entry(PGSTAT_KIND_REPLSLOT, InvalidOid,
- ReplicationSlotIndex(slot)))
+ ReplicationSlotIndex(slot), false))
pgstat_request_entry_refs_gc();
}
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 *shent,
* 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=%s dboid=%u objid=%" PRIu64 " refcount=%u generation=%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 = true;
/* 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 could
* 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 memory
* 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 = {0};
PgStatShared_HashEntry *shent;
@@ -1031,6 +1027,18 @@ pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint64 objid)
shent = 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=%s dboid=%u objid=%" 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 = pgstat_drop_entry_internal(shent, NULL);
/*
diff --git a/src/backend/utils/activity/pgstat_xact.c b/src/backend/utils/activity/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_state, 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_state, 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++;
}
@@ -160,7 +160,7 @@ AtEOSubXact_PgStat_DroppedStats(PgStat_SubXactStatus *xact_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 xl_xact_stats_item *items,
xl_xact_stats_item *it = &items[i];
uint64 objid = ((uint64) it->objid_hi) << 32 | it->objid_lo;
- if (!pgstat_drop_entry(it->kind, it->dboid, objid))
+ if (!pgstat_drop_entry(it->kind, it->dboid, objid, true))
not_freed_count++;
}
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 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)
/* 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();
PG_RETURN_VOID();
--
2.54.0
[application/pgp-signature] signature.asc (833B, ../[email protected]/3-signature.asc)
download
view thread (17+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: BUG #19520: PANIC when concurrently manipulating stored procedures with pg_stat_statements and track_functions =
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox