Fix PANIC with track_functions due to concurrent drop of pgstats entries

pgstat_drop_entry_internal() generates an ERROR if facing a pgstats
entry already marked as dropped.  With a workload doing a lot of
concurrent CALL and DROP/CREATE PROCEDURE, it could be possible for
AtEOXact_PgStat_DroppedStats(), that wants to do transactional drops, to
find entries that are already dropped, after a commit record has been
written.  In this case, ERRORs are upgraded to PANIC, taking down the
server.

This issue is fixed by making pgstat_drop_entry() optionally more
tolerant to concurrent drops, adding to the routine a missing_ok option
to make some of its callers more tolerant (spoiler: some of the callers
want a strict behavior, like replication slots and backend stats).
pgstat_drop_entry_internal() cannot be called anymore for an entry
marked as dropped, hence its error is replaced by an assertion.
Functions are handled as a special case in core; this problem could also
apply to custom stats kinds depending on what an extension does.
track_functions is costly when enabled (disabled by default), which is
perhaps the main reason why this has not be found yet.

A similar version of this patch has been proposed by Sami Imseih on a
different thread for a feature in development.  This version has tweaked
here by me for the sake of fixing this issue.

Reported-by: zhanglihui <zlh21343@163.com>
Author: Sami Imseih <samimseih@gmail.com>
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/19520-73873648d44793cf@postgresql.org
Backpatch-through: 15

diff --git a/src/backend/utils/activity/pgstat_function.c b/src/backend/utils/activity/pgstat_function.c
index a4b26bed5a846dd1c4ea6f01d0ae0f8f33ad4aac..75f3dc31eb8eeb60aa413266a88b2f223dab8de2 100644 (file)
--- 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 8685e23302357f6a257e05543b090c25c4e5ce4e..1fb3ffeedc6b3b8668da4881e4a13e37ec4c0302 100644 (file)
--- a/src/backend/utils/activity/pgstat_replslot.c
+++ b/src/backend/utils/activity/pgstat_replslot.c
@@ -154,7 +154,7 @@ void
 pgstat_drop_replslot(ReplicationSlot *slot)
 {
        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 92e7c83a21425d4825adeaec5b02650649f1ba49..181f9fc701b2483efe2f64ea6580eed201ba59fd 100644 (file)
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -861,13 +861,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 objoid=%u refcount=%u generation=%u",
-                        pgstat_get_kind_info(shent->key.kind)->name,
-                        shent->key.dboid, shent->key.objoid,
-                        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 */
@@ -943,13 +937,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, Oid objoid)
+pgstat_drop_entry(PgStat_Kind kind, Oid dboid, Oid objoid,
+                                 bool missing_ok)
 {
        PgStat_HashKey key;
        PgStatShared_HashEntry *shent;
@@ -977,6 +974,20 @@ pgstat_drop_entry(PgStat_Kind kind, Oid dboid, Oid objoid)
        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 objoid=%u refcount=%u generation=%u",
+                                        pgstat_get_kind_info(shent->key.kind)->name,
+                                        shent->key.dboid,
+                                        shent->key.objoid,
+                                        pg_atomic_read_u32(&shent->refcount),
+                                        pg_atomic_read_u32(&shent->generation));
+                       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 369239d5014c5c8205ce4b1875d16131f5ab7b7a..8f5a90153cfc940657533bc1c86d9c480a05164e 100644 (file)
--- 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, it->objoid))
+                       if (!pgstat_drop_entry(it->kind, it->dboid, it->objoid, 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, it->objoid))
+                       if (!pgstat_drop_entry(it->kind, it->dboid, it->objoid, true))
                                not_freed_count++;
                }
 
@@ -159,7 +159,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, it->objoid))
+                       if (!pgstat_drop_entry(it->kind, it->dboid, it->objoid, true))
                                not_freed_count++;
                        pfree(pending);
                }
@@ -321,7 +321,7 @@ pgstat_execute_transactional_drops(int ndrops, struct xl_xact_stats_item *items,
        {
                xl_xact_stats_item *it = &items[i];
 
-               if (!pgstat_drop_entry(it->kind, it->dboid, it->objoid))
+               if (!pgstat_drop_entry(it->kind, it->dboid, it->objoid, true))
                        not_freed_count++;
        }
 

diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index f886ab7f4bc11f8a2bd7cf886b30a71673fa2f82..b856fbc6ab0c4e6ecd87dc36812482dff58a05f1 100644 (file)
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -625,7 +625,8 @@ extern PgStat_EntryRef *pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, Oid ob
 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, Oid objoid);
+extern bool pgstat_drop_entry(PgStat_Kind kind, Oid dboid, Oid objoid,
+                                                         bool missing_ok);
 extern void pgstat_drop_all_entries(void);
 extern PgStat_EntryRef *pgstat_get_entry_ref_locked(PgStat_Kind kind, Oid dboid, Oid objoid,
                                                                                                        bool nowait);