Re: Memory consumed by child SpecialJoinInfo in partitionwise join planning

On Mon, Mar 18, 2024 at 20:11 Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>
wrote:

> Hi Amit,
>
>
> On Fri, Mar 15, 2024 at 11:45 AM Amit Langote <amitlangote09(at)gmail(dot)com>
> wrote:
>
>> > >
>> > > That being said I'm a big fan of using a local variable on stack and
>> > > filling it. I'd probably go with the usual palloc/pfree, because that
>> > > makes it much easier to use - the callers would not be responsible for
>> > > allocating the SpecialJoinInfo struct. Sure, it's a little bit of
>> > > overhead, but with the AllocSet caching I doubt it's measurable.
>> >
>> > You are suggesting that instead of declaring a local variable of type
>> > SpecialJoinInfo, build_child_join_sjinfo() should palloc() and return
>> > SpecialJoinInfo which will be freed by free_child_sjinfo()
>> > (free_child_sjinfo_members in the patch). I am fine with that.
>>
>> Agree with Tomas about using makeNode() / pfree(). Having the pfree()
>> kind of makes it extra-evident that those SpecialJoinInfos are
>> transitory.
>>
>
> Attached patch-set
>
> 0001 - original patch as is
> 0002 - addresses your first set of comments
> 0003 - uses palloc and pfree to allocate and deallocate SpecialJoinInfo
> structure.
>
> I will squash both 0002 and 0003 into 0001 once you review those changes
> and are fine with those.
>

Thanks for the new patches.

> > I did put this through check-world on amd64/arm64, with valgrind,
>> > > without any issue. I also tried the scripts shared by Ashutosh in his
>> > > initial message (with some minor fixes, adding MEMORY to explain etc).
>> > >
>> > > The results with the 20240130 patches are like this:
>> > >
>> > > tables master patched
>> > > -----------------------------
>> > > 2 40.8 39.9
>> > > 3 151.7 142.6
>> > > 4 464.0 418.5
>> > > 5 1663.9 1419.5
>>
>> Could you please post the numbers with the palloc() / pfree() version?
>>
>>
> Here are they
> tables | master | patched
> --------+---------+---------
> 2 | 29 MB | 28 MB
> 3 | 102 MB | 93 MB
> 4 | 307 MB | 263 MB
> 5 | 1076 MB | 843 MB
>
> The numbers look slightly different from my earlier numbers. But they were
> quite old. The patch used to measure memory that time is different from the
> one that we committed. So there's a slight difference in the way we measure
> memory as well.
>

Sorry, I should’ve mentioned that I was interested in seeing cpu times to
compare the two approaches. Specifically, to see if the palloc / frees add
noticeable overhead.