Re: Improve monitoring of shared memory allocations

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