Re: Improve monitoring of shared memory allocations

From: Tomas Vondra <tomas(at)vondra(dot)me>
To: Rahila Syed <rahilasyed90(at)gmail(dot)com>
Cc: Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Improve monitoring of shared memory allocations
Date: 2025-03-31 14:11:39
Message-ID: 7987a0b5-9933-457a-b5ba-d28e9cce883b@vondra.me
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 3/31/25 01:01, Rahila Syed wrote:
> ...
>
>
> > I will improve the comment in the next version.
> >
>
> OK. Do we even need to pass nelem_alloc to hash_get_init_size? It's not
> really used except for this bit:
>
> +    if (init_size > nelem_alloc)
> +        element_alloc = false;
>
> Can't we determine before calling the function, to make it a bit less
> confusing?
>
>
> Yes, we could determine whether the pre-allocated elements are zero before
> calling the function, I have fixed it accordingly in the attached 0001
> patch. 
> Now, there's no need to pass `nelem_alloc` as a parameter. Instead, I've
> passed this information as a boolean variable-initial_elems. If it is
> false,
> no elements are pre-allocated.
>
> Please find attached the v7-series, which incorporates your review patches
> and addresses a few remaining comments.
>

I think it's almost committable. Attached is v8 with some minor review
adjustments, and updated commit messages. Please read through those and
feel free to suggest changes.

I still found the hash_get_init_size() comment unclear, and it also
referenced init_size, which is no longer relevant. I improved the
comment a bit (I find it useful to mimic comments of nearby functions,
so I did that too here). The "initial_elems" name was a bit confusing,
as it seemed to suggest "number of elements", but it's a simple flag. So
I renamed it to "prealloc", which seems clearer to me. I also tweaked
(reordered/reformatted) the conditions a bit.

For the other patch, I realized we can simply MemSet() the whole chunk,
instead of resetting the individual parts.

regards

--
Tomas Vondra

Attachment Content-Type Size
v8-0001-Improve-acounting-for-memory-used-by-shared-hash-.patch text/x-patch 16.6 KB
v8-0002-review.patch text/x-patch 6.7 KB
v8-0003-Improve-accounting-for-PredXactList-RWConflictPoo.patch text/x-patch 7.4 KB
v8-0004-review.patch text/x-patch 1.6 KB
v8-0005-Add-cacheline-padding-between-heavily-accessed-ar.patch text/x-patch 2.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Antonin Houska 2025-03-31 14:26:25 Re: why there is not VACUUM FULL CONCURRENTLY?
Previous Message Yugo NAGATA 2025-03-31 14:09:56 Re: Allow ILIKE forward matching to use btree index