Re: pgstat: don't assert on dropped entries when writing stats file

> > 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)