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.
