Re: BF mamba failure

On Wed, Oct 16, 2024 at 10:11:08AM +0000, Bertrand Drouvot wrote:
> Indeed, in pgstat_release_entry_ref(), we're doing:
> 
> if (pg_atomic_fetch_sub_u32(&entry_ref->shared_entry->refcount, 1) == 1)
> .
> .
> 	shent = dshash_find(pgStatLocal.shared_hash,
>                         &entry_ref->shared_entry->key,
>                         true);
> 
> I wonder if we are not decrementing &entry_ref->shared_entry->refcount too early.
> 
> I mean, wouldn't that make sense to decrement it after the dshash_find() call?
> (to ensure a "proper" whole entry locking, done in dshash_find(), first)

Making that a two-step process (first step to read the refcount,
second step to decrement it after taking the exclusive lock through  
dshash_find) would make the logic more complicated what what we have
now, without offering more protection, afaik, because you'd still need
to worry about a race condition between the first and second steps.
Making this a one-step only would incur more dshash_find() calls than
we actually need, and I would not underestimate that under a
heavily-concurrent pgstat_release_entry_ref() path taken.

> Yeah, FWIW, we would be going from 32 bytes:
>                                /* total size (bytes):   32 */
> 
> to 40 bytes (with the patch applied):
>                                /* total size (bytes):   40 */
> 
> Due to the padding, that's a 8 bytes increase while we're adding "only" 4 bytes.

I have spent some time digging into all the original pgstat threads 
dealing with the shmem implementation or dshash, and I'm not really
seeing anybody stressing on the size of the individual hash entries.
This stuff was already wasting space with padding originally, so
perhaps we're stressing too much here?  Would anybody like to comment?
--
Michael