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 |
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 |