Re: BF mamba failure

Hi,

On Wed, Oct 16, 2024 at 09:43:48AM +0900, Michael Paquier wrote:
> On Fri, Oct 11, 2024 at 08:18:58AM +0000, Bertrand Drouvot wrote:
> > On Fri, Oct 11, 2024 at 11:07:29AM +0300, Kouber Saparev wrote:
> >> Unfortunately not, we are running 15.4 and planning to upgrade very soon.
> >> Is the patch mentioned already merged in PostgreSQL 16?
> > 
> > Yes, as of 16.4.
> 
> Right.  I'd surely welcome more eyes on what I have posted here:
> https://www.postgresql.org/message-id/Zm-8Xo93K9yD9fy7@paquier.xyz

I applied your patch and do confirm that it fixes the issue. While that works I
wonder is there no other way to fix the issue.

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)

> I am a bit annoyed by the fact of making PgStatShared_HashEntry
> slightly larger to track the age of the entries,

Yeah, FWIW, we would be going from 32 bytes:

(gdb)  ptype /o struct PgStatShared_HashEntry
/* offset      |    size */  type = struct PgStatShared_HashEntry {
/*      0      |      16 */    PgStat_HashKey key;
/*     16      |       1 */    _Bool dropped;
/* XXX  3-byte hole      */
/*     20      |       4 */    pg_atomic_uint32 refcount;
/*     24      |       8 */    dsa_pointer body;

                               /* total size (bytes):   32 */
                             }

to 40 bytes (with the patch applied):

(gdb) ptype /o struct PgStatShared_HashEntry
/* offset      |    size */  type = struct PgStatShared_HashEntry {
/*      0      |      16 */    PgStat_HashKey key;
/*     16      |       1 */    _Bool dropped;
/* XXX  3-byte hole      */
/*     20      |       4 */    pg_atomic_uint32 refcount;
/*     24      |       4 */    pg_atomic_uint32 agecount;
/* XXX  4-byte hole      */
/*     32      |       8 */    dsa_pointer body;

                               /* total size (bytes):   40 */
                             }

Due to the padding, that's a 8 bytes increase while we're adding "only" 4 bytes.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com