On Mon, Mar 18, 2024 at 5:05 PM Amit Langote <amitlangot...@gmail.com>
wrote:
> On Mon, Mar 18, 2024 at 20:11 Ashutosh Bapat <ashutosh.bapat....@gmail.com>
> wrote:
>
>> Hi Amit,
>>
>>
>> On Fri, Mar 15, 2024 at 11:45 AM Amit Langote <amitlangot...@gmail.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.
>
No problem. Here you go
tables | master | patched | perc_change
--------+----------+----------+-------------
2 | 477.87 | 492.32 | -3.02
3 | 1970.83 | 1989.52 | -0.95
4 | 6305.01 | 6268.81 | 0.57
5 | 19261.56 | 18684.86 | 2.99
+ve change indicates reduced planning time. It seems that the planning time
improves as the number of tables increases. But all the numbers are well
within noise levels and thus may not show any improvement or deterioration.
If you want, I can repeat the experiment.
--
Best Wishes,
Ashutosh Bapat
