> > Done. v4 is split into two patches.
>
> I have spent some time on that, still in the middle of it, and for now
> I am attaching my edits as of a v5-0003 that can apply on top of your
> set of two patches (re-attached, these are untouched).
Thanks!
v6 brings in your edits. I agree with them. I also made some minor
comment changes.
The other substantial changes are:
v6-0001:
1/
@@ -1062,9 +1178,15 @@ pgstat_drop_entry(PgStat_Kind kind, Oid dboid,
uint64 objid,
bool missing_ok)
{
PgStat_HashKey key = {0};
+ dshash_table *hash;
PgStatShared_HashEntry *shent;
bool freed = true;
+ Assert(kind >= PGSTAT_KIND_MIN && kind <= PGSTAT_KIND_MAX);
+ Assert(pgStatLocal.kind_hash[kind] != NULL);
+
+ hash = pgStatLocal.kind_hash[kind];
+
These asserts should be an ERROR instead. pgstat_drop_entry() can be called
before the kind is registered, as is the case in
pgstat_execute_transactional_drops.
For example if a custom stats extension is removed from config. In that case
kind_hash[kind] is NULL, and this would dereference it before
missing_ok is reached.
I changed this to return true when missing_ok is set, and ERROR
otherwise. I could
not find any other paths that need this handling.
Of course, there is also the alternate case where we associate a kind
id with a completely
different kind on startup, and in that case, it's ok to drop the
entries. All WAL/2PC records
store is a kind_id, and I don't think we need anything more complex.
2/
Renamed all_hashes/num_hashes to var_hashes/num_var_hashes.
3/
Removed test_custom_stats_var_is_own_hash(). I agree with you. I was thinking
about this one yesterday, but we have enough safeguards in place.
v6-0002:
4/ The existing TAP test was missing the own_hash column.
--
Sami
