On Mon, May 17, 2021 at 7:52 AM Nitin Jadhav <nitinjadhavpostg...@gmail.com> wrote:
> > > While working on [1], I observed that extra memory is allocated in > > > [1] > https://mail.google.com/mail/u/2/#search/multi+column+list/KtbxLxgZZTjRxNrBWvmHzDTHXCHLssSprg?compose=CllgCHrjDqKgWCBNMmLqhzKhmrvHhSRlRVZxPCVcLkLmFQwrccpTpqLNgbWqKkTkTFCHMtZjWnV > > I am really sorry for this. I wanted to point to the thread subjected > to 'Multi-Column List Partitioning'. > > > If it's worth counting list elements in advance, then you can also > allocate the > > PartitionListValue as a single chunk, rather than palloc in a loop. > > This may help large partition heirarchies. > > > > And the same thing in create_hash_bounds with hbounds. > > I agree and thanks for creating those patches. I am not able to apply > the patch on the latest HEAD. Kindly check and upload the modified > patches. > > > I'm not able to detect that this is saving more than about ~1% less RAM, > to > > create or select from 1000 partitions, probably because other data > structures > > are using much more, and savings here are relatively small. > > Yes it does not save huge memory but it's an improvement. > > > I'm going to add to the next CF. You can add yourself as an author, and > watch > > that the patch passes tests in cfbot. > > https://commitfest.postgresql.org/ > > http://cfbot.cputube.org/ > > Thanks for creating the commitfest entry. > > > Since the function returns the total number of non null bound values, > should it be named get_non_null_list_bounds_count ? > > It can also be named get_count_of_... but that's longer. > > Changed it to 'get_non_null_list_bounds_count'. > > > The palloc() call would take place even if ndatums is 0. I think in that > case, palloc() doesn't need to be called. > > I feel there is no such case where the 'ndatums' is 0 because as we > can see below, there is an assert in the 'partition_bounds_create' > function from where we call the 'create_list_bounds' function. Kindly > provide such a case if I am wrong. > > PartitionBoundInfo > partition_bounds_create(PartitionBoundSpec **boundspecs, int nparts, > PartitionKey key, int **mapping) > { > int i; > > Assert(nparts > 0); > Hi, Thanks for pointing out the assertion. My corresponding comment can be dropped. Cheers > > Thanks & Regards, > Nitin Jadhav > On Sun, May 16, 2021 at 10:52 PM Zhihong Yu <z...@yugabyte.com> wrote: > > > > > > > > On Sun, May 16, 2021 at 10:00 AM Justin Pryzby <pry...@telsasoft.com> > wrote: > >> > >> On Sat, May 15, 2021 at 02:40:45PM +0530, Nitin Jadhav wrote: > >> > While working on [1], I observed that extra memory is allocated in > >> > [1] > https://mail.google.com/mail/u/2/#search/multi+column+list/KtbxLxgZZTjRxNrBWvmHzDTHXCHLssSprg?compose=CllgCHrjDqKgWCBNMmLqhzKhmrvHhSRlRVZxPCVcLkLmFQwrccpTpqLNgbWqKkTkTFCHMtZjWnV > >> > >> This is a link to your gmail, not to anything public. > >> > >> If it's worth counting list elements in advance, then you can also > allocate the > >> PartitionListValue as a single chunk, rather than palloc in a loop. > >> This may help large partition heirarchies. > >> > >> And the same thing in create_hash_bounds with hbounds. > >> > >> create_range_bounds() already doesn't call palloc in a loop. However, > then > >> there's an asymmetry in create_range_bounds, which is still takes a > >> double-indirect pointer. > >> > >> I'm not able to detect that this is saving more than about ~1% less > RAM, to > >> create or select from 1000 partitions, probably because other data > structures > >> are using much more, and savings here are relatively small. > >> > >> I'm going to add to the next CF. You can add yourself as an author, > and watch > >> that the patch passes tests in cfbot. > >> https://commitfest.postgresql.org/ > >> http://cfbot.cputube.org/ > >> > >> Thanks, > >> -- > >> Justin > > > > Hi, > > For 0001-Removed-extra-memory-allocations-from-create_list_bo.patch : > > > > +static int > > +get_non_null_count_list_bounds(PartitionBoundSpec **boundspecs, int > nparts) > > > > Since the function returns the total number of non null bound values, > should it be named get_non_null_list_bounds_count ? > > It can also be named get_count_of_... but that's longer. > > > > + all_values = (PartitionListValue **) > > + palloc(ndatums * sizeof(PartitionListValue *)); > > > > The palloc() call would take place even if ndatums is 0. I think in that > case, palloc() doesn't need to be called. > > > > Cheers > > >
