public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Bertrand Drouvot <[email protected]>
Cc: Kouber Saparev <[email protected]>
Cc: [email protected]
Subject: Re: BF mamba failure
Date: Thu, 17 Oct 2024 13:24:50 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CAA4eK1KxuMVyAryz_Vk5yq3ejgKYcL6F45Hj9ZnMNBS-g+PuZg@mail.gmail.com>
<[email protected]>
<172854117093.1162.5418906857588663992.pgcf@coridan.postgresql.org>
<[email protected]>
<CAN4RuQtMHbVjWX-0sHOmVUxpsbMDX0tMndvfmNk-+ak4QBUWJg@mail.gmail.com>
<ZwjfckL/[email protected]>
<[email protected]>
<[email protected]>
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../[email protected]/2-signature.asc)
download
view thread (4+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: BF mamba failure
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox