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
signature.asc
Description: PGP signature
