public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tomas Vondra <[email protected]>
To: Rahila Syed <[email protected]>
Cc: Nazir Bilal Yavuz <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Improve monitoring of shared memory allocations
Date: Thu, 27 Mar 2025 13:56:08 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAH2L28uG_g1Ljo8aL-g1MupJXO4Y7-a-bUCriE7w2213+KSGdA@mail.gmail.com>
References: <CAH2L28vHzRankszhqz7deXURxKncxfirnuW68zD7+hVAqaS5GQ@mail.gmail.com>
<k6f6ynjvb7lvebhygaiqsfrdohq672uughk3q4ve4q5jqljywz@7jbrz724epeq>
<CAH2L28uL-8EQTRSeyTpW1DqAXsDXRkXCkT1dus2u6p4HYDrxAg@mail.gmail.com>
<CAH2L28uGLhkXBKDWFKm5XZtp_0nNqpYQ3Hc35vG++mM7wuOhgg@mail.gmail.com>
<CAN55FZ3cJxy0VkeXpuO3K4BpjzJo3S6oU+iMyc00P6gEjqPztw@mail.gmail.com>
<CAH2L28uwxJREzB62UjRDBumE87hHWUJJvRwxqqbO+7qFmoZfTg@mail.gmail.com>
<[email protected]>
<CAH2L28uG_g1Ljo8aL-g1MupJXO4Y7-a-bUCriE7w2213+KSGdA@mail.gmail.com>
Hi,
On 3/27/25 13:02, Rahila Syed wrote:
>
> Hi Tomas,
>
>
> I did a review on v3 of the patch. I see there's been some minor changes
> in v4 - I haven't tried to adjust my review, but I assume most of my
> comments still apply.
>
>
> Thank you for your interest and review.
>
>
> 1) I don't quite understand why hash_get_shared_size() got renamed to
> hash_get_init_size()? Why is that needed? Does it cover only some
> initial allocation, and there's additional memory allocated later? And
> what's the point of the added const?
>
>
> Earlier, this function was used to calculate the size for shared hash
> tables only.
> Now, it also calculates the size for a non-shared hash table. Hence the
> change
> of name.
>
> If I don't change the argument to const, I get a compilation error as
> follows:
> "passing argument 1 of ‘hash_get_init_size’ discards ‘const’ qualifier
> from pointer target type."
> This is due to this function being called from hash_create which
> considers HASHCTL to be
> a const.
>
OK, makes sense. I haven't tried removing the const, but if removing it
would trigger a compiler warning, it's probably needed.
> ...
>
> FWIW I see no justification for why the cache line padding is useful,
> and it seems quite unlikely this would benefit anything, consiering it
> adds padding between fairly long arrays. What kind of contention can we
> get there? I don't get it.
>
>
> I have done that to address a review comment upthread by Andres and
> the because comment above that code block also mentions need for
> padding.
>
Neither that seems like a sufficient justification for the change. The
comment is more of a speculation, it doesn't demonstrate the benefits
are real. It's true Andres tends to be right about these things, but
still, I'd like to see some argument for why this helps.
How exactly does padding before/after the whole array help anything?
In fact, is this the padding Andres (or the comment) suggested? The
comment says:
* XXX: It might make sense to increase padding for these arrays, given
* how hotly they are accessed.
Doesn't "padding of array" actually suggest padding each element, not
the array as a whole?
In any case, if the 0003 patch addresses the padding, it should also
remove the XXX comment.
>
> Also, why is the patch adding padding after statusFlags (the last array
> allocated in InitProcGlobal) and not between allProcs and xids?
>
>
> I agree it makes more sense this way, so changing accordingly.
>
>
>
> *
> + * XXX can we rely on this matching the calculation
> in hash_get_shared_size?
> + * or could/should we add some asserts? Can we have
> at least some sanity checks
> + * on nbuckets/nsegs?
>
>
> Both places rely on compute_buckets_and_segs() to calculate nbuckets
> and nsegs,
> hence the probability of mismatch is low. I am open to adding some
> asserts to verify this.
> Do you have any suggestions in mind?
>
> Please find attached updated patches after merging all your review
> comments except
> a few discussed above.
>
OK, I don't have any other comments for 0001 and 0002. I'll do some
more review and polishing on those, and will get them committed soon.
I don't plan to push 0003, unless someone can actually explain and
demonstrate the benefits of the proposed padding,
regards
--
Tomas Vondra
view thread (20+ 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: Improve monitoring of shared memory allocations
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