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