pgstat: don't assert on dropped entries when writing stats file
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t254237psql -h localhost -U postgresBuilt from patchset v3 (message #3), October 20, 2026 at 03:43 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t254237_3 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t254237_3 && git checkout t254237_3Patchset v3 (message #3) is on t254237_3
Hi,
I ran into a small pgstat assertion failure while looking at this code
recently.
Here is a simple repro. Set wal_level = logical, then:
Session 1:
```
SELECT * FROM pg_create_logical_replication_slot('slot_a', 'pgoutput');
-- keep the session open
```
Session 2:
```
SELECT pg_drop_replication_slot('slot_a');
SELECT * FROM pg_create_logical_replication_slot('slot_b', 'pgoutput');
SELECT pg_drop_replication_slot('slot_b');
\q
```
Session 1:
```
\q
```
Then restart:
```
pg_ctl -m fast -w restart
```
With assertions enabled, the shutdown checkpoint hits:
```
TRAP: failed Assert("!ps->dropped"), File:
"../src/backend/utils/activity/pgstat.c", Line: 1774, PID: 2117776
postgres: checkpointer (ExceptionalCondition+0xab)[0xbf3f7c]
postgres: checkpointer [0xa46128]
postgres: checkpointer (pgstat_before_server_shutdown+0xcc)[0xa43d89]
postgres: checkpointer (shmem_exit+0xaa)[0x9d8be1]
postgres: checkpointer (proc_exit+0x71)[0x9d89b1]
```
I think the assertion is too strong. Dropped variable-numbered stats entries
can stay in the hash table while references to them still exist. Replication
slot stats make this easy to hit because slot_b can reuse slot_a's slot
index. The stats are serialized by name on disk, but use the slot index as
their key while running.
The old comment made this sound like something that should not happen, or at
least like some cleanup went sideways. But this case can happen without
anything going wrong.
We could probably fix this in the reinitialization path instead, but
that feels like a lot of complexity for a state we can already tolerate.
pgstat_write_statsfile() already skips dropped entries before serializing
them, so the patch just removes the assertion and updates the comment to
match that.
--
Sami Imseih
Amazon Web Services (AWS)
On Fri, Oct 16, 2026 at 10:53:01AM -0500, Sami Imseih wrote:
I ran into a small pgstat assertion failure while looking at this code
recently.
I've been wondering for some time if this could be reached with a set
of deterministic steps, but never got down to track if there were such
a sequence.
The old comment made this sound like something that should not happen, or at
least like some cleanup went sideways. But this case can happen without
anything going wrong.We could probably fix this in the reinitialization path instead, but
that feels like a lot of complexity for a state we can already tolerate.
pgstat_write_statsfile() already skips dropped entries before serializing
them, so the patch just removes the assertion and updates the comment to
match that.
Hmm. This assertion has proved to be useful in the past, to be able
to track stale entry references in the hash table where we don't
expect them, pointing at some real bugs. See for example
da99fedf8c17.
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.
--
Michael
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
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.
Good point on the elog(ERROR). If another backend reinitializes and
drops the same key after we decrement the refcount to 0 but before
dshash_find(), then a missing entry is valid and should not fail. That
is a very narrow window, but your stress test shows that it is possible.
- 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.
I am not sure we need the generation check here:
+ 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);
+ }Generation does not really tell us that this is another entry created
with the same key. It could be the same entry with the same generation,
right?
So, I don't think we need to inspect generation here. If the entry is
dropped and refcount is 0, free it. Otherwise just release the lock.
Maybe something like:
+ else
+ {
+ /* Entry reinitialized or created again, still in use. */
+
dshash_release_lock(pgStatLocal.kind_hash[key.kind], shent);
+ }Anyhow, the else-if and else branches do the same thing as it stands.
--
Sami Imseih
Amazon Web Services (AWS)
On Tue, Oct 20, 2026 at 11:07:21AM -0500, Sami Imseih wrote:
Generation does not really tell us that this is another entry created
with the same key. It could be the same entry with the same generation,
right?
I was doing a bit more testing, and hitting the generation check is
possible, but in all of them I just get generation == 0, pointing at
an entirely new entry. I'll drop this part.
+ else + { + /* Entry reinitialized or created again, still in use. */ + dshash_release_lock(pgStatLocal.kind_hash[key.kind], shent); + }Anyhow, the else-if and else branches do the same thing as it stands.
Yep, makes sense.
I am also attaching an equivalent of what I have used to stress-test
the entry releases. At first, I was using a set of terminals with
pgbench commands running in an infinite loop and server restarts.
That was annoying, so I've asked Claude to summarize that into a
single equivalent script that can be reused at will. If you want to
enlarge the window, add a small pg_usleep() before the dshash_find()
that does the exclusive lock lookup, like 20ms. Without the hardcoded
sleep, it takes a few more iterations to hit the various failures.
--
Michael