Re: Improve monitoring of shared memory allocations

Hi, I'm sorry, but I'm afraid this won't make it into PG18 :-( AFAICS the updated patch is correct / not buggy for non-shared hash tables, but considering I missed a pretty serious flaw before pushing the original patch, and that the tests didn't catch that either ... The risk/benefit is not rea

Re: Improve monitoring of shared memory allocations

Thanks! I've pushed the first two parts, improving the shmem accounting. I've done a couple additional adjustments before commit, namely: 1) I've renamed hash_get_init_size() to just hash_get_size(). We've not indicated that it's "initial" allocation before, I don't think we need to indicate to

Re: Improve monitoring of shared memory allocations

Hi, 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. > > The changes look good to me. About the following question. /* XXX what about segment size? should check have H

Re: Improve monitoring of shared memory allocations

Hi, Analysis of the Bug in 0002 reported by David Rowley : The 0001* patch allocates memory for the hash header, directory, segments, and elements collectively for both shared and non-shared hash tables. While

Re: Improve monitoring of shared memory allocations

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_

Re: Improve monitoring of shared memory allocations

Hi Tomas, > > Right. I'm still not convinced if this makes any difference, or whether > this alignment was merely a consequence of using ShmemAlloc(). I don't > want to make this harder to understand unnecessarily. > Yeah, it makes sense. > Let's keep this simple - without additional alignment

Re: Improve monitoring of shared memory allocations

On 3/28/25 12:10, Rahila Syed wrote: > Hi Tomas, > > > 1) alignment > > There was a comment with a question whether we need to MAXALIGN the > chunks in dynahash.c, which were originally allocated by ShmemAlloc, but > now it's part of one large allocation, which is then cut int

Re: Improve monitoring of shared memory allocations

Hi Tomas, 1) alignment > > There was a comment with a question whether we need to MAXALIGN the > chunks in dynahash.c, which were originally allocated by ShmemAlloc, but > now it's part of one large allocation, which is then cut into pieces > (using pointer arithmetics). > > I was not sure whethe

Re: Improve monitoring of shared memory allocations

On 3/27/25 13:56, Tomas Vondra wrote: > ... > > 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. > Actually ... while polishing 0001 and 0002, I noticed a couple more details that I'd like to ask about.

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

Re: Improve monitoring of shared memory allocations

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

Re: Improve monitoring of shared memory allocations

Hi, 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. Most of my suggestions are about formatting and/or readability. Some of the likes (e.g. the pointer arithmetics) got so long pgin

Re: Improve monitoring of shared memory allocations

Hi Bilal, I have a couple of comments, I have only reviewed 0001 so far. > Thank you for reviewing! > > You may need to run pgindent, it makes some changes. > Attached v4-patch has been updated after running pgindent. + * If table is shared, calculate the offset at which to find the

Re: Improve monitoring of shared memory allocations

Hi, On Wed, 12 Mar 2025 at 13:46, Rahila Syed wrote: > I have now made the changes uniformly across shared and non-shared hash > tables, > making the code fix look cleaner. I hope this aligns with your suggestions. > Please find attached updated and rebased versions of both patches. Thank you f

Re: Improve monitoring of shared memory allocations

Hi Andres, >> > + if (hashp->isshared) >> > + { >> > + int nsegs; >> > + int nbuckets; >> > + nsegs = find_num_of_segs(nelem, &nbuckets, >> hctl->num_partitions, hctl->ssiz

Re: Improve monitoring of shared memory allocations

Hi, Thank you for the review. cfbot found a few compiler warnings: > > https://cirrus-ci.com/task/6526903542087680 > [16:47:46.964] make -s -j${BUILD_JOBS} clean > [16:47:47.452] time make -s -j${BUILD_JOBS} world-bin > [16:49:10.496] lwlock.c: In function ‘CreateLWLocks’: > [16:49:10.496] lwloc

Re: Improve monitoring of shared memory allocations

Hi, Thanks for sending these, the issues addressed here have been bugging me for a long while. On 2025-03-01 10:19:01 +0530, Rahila Syed wrote: > The 0001* patch improved the accounting for the shared memory allocated for > a hash table during hash_create. pg_shmem_allocations tracks the memory

Improve monitoring of shared memory allocations

Hi, The 0001* patch improved the accounting for the shared memory allocated for a hash table during hash_create. pg_shmem_allocations tracks the memory allocated by ShmemInitStruct, which, for shared hash tables, only covers memory allocated for the hash directory and control structure via ShmemIn