On Mon, Oct 19, 2026 at 02:52:41PM +0900, Michael Paquier wrote: > On top of the generation check, how about doing something in > pgstat_release_entry_ref() if we have a dropped entry with a refcount > of 0 and the generation does not match? Hence, I'd suggest that we > should free the entry more aggressively, perhaps.. I have not tested > this idea, but just getting rid of this assertion does not feel like > the right move to me as it means that we leave in the shared hashtable > entries that should be gone, and that these entries will never be gone > until the same key is reused. For the replication slots or an > index-based logic, the turnover should be quick, but that may not be > the case for all stats kinds.
I have done some testing by reusing your test case with pgbench and a combination of things, like: - slot peeks and sleeps to maintain references - Concurrent activity creates and drops the slots - Hardcoded sleep in pgstat_release_entry_ref() before the dshash_find() to let concurrent activity mess more easily with a backend doing a release. And this has uncovered two more things, pointing at real issues that your assertion was a consequence of: - elog(ERROR) could be hit during backend exit, due to a concurrent reinit and drop. We need to make the code slightly sloppier, that should not fail. - The generation match check should be a secondary check, after checking that the entry has not been marked for drop. That could trigger the assertion. On top of the concurrent pgbenches, I have added some periodic restarts in the middle of the runs, and the assertion failures can be hit pretty quickly on HEAD. None of them show up with the patch attached. There is also a TAP test able to trigger your assertion on HEAD, based on what you have sent. Thoughts? -- Michael
From 04c77bcd3e877ccf50173bb96768d822a5c265ce Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Tue, 20 Oct 2026 12:25:02 +0900 Subject: [PATCH] Fix cleanup of reused pgstats entries in pgstat_release_entry_ref() When a stats entry is reinitialized and dropped again while a backend still holds a reference from an older generation, the backend releasing the last reference found a generation mismatch and left behind an entry marked as dropped with a refcount of 0. Nothing would free it, and the checkpointer would trigger an assertion at shutdown when flushing the stats. Looking more at the issue, using some hardcoded sleeps before the dshash_find() that takes an exclusive lock when releasing an entry with some concurrent activity, there was more going on: - A missing entry could be reported as an error at backend exit, error that should not be hit even if the entry was reinitialized and dropped concurrently. - The generation check was not correct for the case of 0 due to the first issue. A generation of 0 refers to an entry that has been freshly created. This commit reworks the entry release to be safer concurrent-wise when a entry targetted for release is found as already existing: - If marked as dropped without refount at 0, free it. - If generation does not match, the entry has been created again with the same key, ignore it. - In other cases, ignore it. The entry has been reinitialized, and is still in use. Reported-by: Sami Imseih <[email protected]> Discussion: https://postgr.es/m/ Backpatch-through: 15 --- src/backend/utils/activity/pgstat_shmem.c | 49 +++++++++-------------- contrib/test_decoding/t/001_repl_stats.pl | 38 ++++++++++++++++++ 2 files changed, 58 insertions(+), 29 deletions(-) diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c index 9b571013dbd3..4f1e857c5814 100644 --- a/src/backend/utils/activity/pgstat_shmem.c +++ b/src/backend/utils/activity/pgstat_shmem.c @@ -800,38 +800,29 @@ pgstat_release_entry_ref(PgStat_HashKey key, PgStat_EntryRef *entry_ref, PgStatShared_HashEntry *shent; /* - * We're the last referrer to this entry, try to drop the shared - * entry. + * We're the last referrer. Before we get the lock, the entry may + * have been reinitialized, or freed and created again, so look it + * up by key. If it is gone, it got already dropped and freed. */ + shent = dshash_find(pgStatLocal.kind_hash[key.kind], &key, true); - /* only dropped entries can reach a 0 refcount */ - Assert(entry_ref->shared_entry->dropped); - - shent = dshash_find(pgStatLocal.kind_hash[key.kind], - &entry_ref->shared_entry->key, true); - if (!shent) - elog(ERROR, "could not find just referenced shared stats entry"); - - /* - * This entry may have been reinitialized while trying to release - * it, so double-check that it has not been reused while holding a - * lock on its shared entry. - */ - if (pg_atomic_read_u32(&entry_ref->shared_entry->generation) == - entry_ref->generation) + if (shent) { - /* Same "generation", so we're OK with the removal */ - Assert(pg_atomic_read_u32(&entry_ref->shared_entry->refcount) == 0); - Assert(entry_ref->shared_entry == shent); - pgstat_free_entry(shent, NULL); - } - else - { - /* - * Shared stats entry has been reinitialized, so do not drop - * its shared entry, only release its lock. - */ - dshash_release_lock(pgStatLocal.kind_hash[key.kind], shent); + /* Free the entry if dropped and unreferenced. */ + if (shent->dropped && + pg_atomic_read_u32(&shent->refcount) == 0) + pgstat_free_entry(shent, NULL); + else if (pg_atomic_read_u32(&shent->generation) == + entry_ref->generation) + { + /* Another entry, created again with the same key. */ + dshash_release_lock(pgStatLocal.kind_hash[key.kind], shent); + } + else + { + /* Entry reinitialized, still in use. */ + dshash_release_lock(pgStatLocal.kind_hash[key.kind], shent); + } } } } diff --git a/contrib/test_decoding/t/001_repl_stats.pl b/contrib/test_decoding/t/001_repl_stats.pl index 6814c792e2b6..47e26daba88f 100644 --- a/contrib/test_decoding/t/001_repl_stats.pl +++ b/contrib/test_decoding/t/001_repl_stats.pl @@ -178,4 +178,42 @@ ok(-f "$stats_file", "stats file must exist after shutdown"); $bpgsql->quit; +# Session holding a stale reference to a slot stats entry, reinitialized +# then dropped again. It must be freed on exit. +$node->start; + +my $slot_name_stale = 'regression_slot6'; +$node->safe_psql('postgres', + "SELECT pg_create_logical_replication_slot('$slot_name_stale', 'test_decoding');" +); + +$bpgsql = $node->background_psql('postgres', on_error_stop => 1); +$bpgsql->query_safe( + "SELECT pg_logical_slot_peek_binary_changes('$slot_name_stale', NULL, NULL)" +); + +# Entry dropped, still kept by the session with a reference. +$node->safe_psql('postgres', + "SELECT pg_drop_replication_slot('$slot_name_stale')"); + +# Entry reinitialized for the new slot, then dropped again. The session +# still holds its reference. +$node->safe_psql( + 'postgres', qq[ + SELECT pg_create_logical_replication_slot('regression_slot7', 'test_decoding'); + SELECT pg_drop_replication_slot('regression_slot7'); +]); + +# This session is the last referrer, and frees the entry on exit. +$bpgsql->quit; + +$node->stop; + +command_like( + [ 'pg_controldata', $node->data_dir ], + qr/Database cluster state:\s+shut down\n/, + 'node shut down ok with stale stats reference'); +ok(-f "$stats_file", + "stats file must exist after shutdown with stale stats reference"); + done_testing(); -- 2.56.0
signature.asc
Description: PGP signature
