Thanks for repeating the benchmark. So I don't see a problem with keeping the existing palloc() way of allocating the SpecialJoinInfos(). We're adding a few cycles by adding free_child_join_sjinfo() (see my delta patch about the rename), but the reduction in memory consumption due to it, which is our goal here, far outweighs what looks like a minor CPU slowdown.
I have squashed 0002, 0003 into 0001, done some changes of my own, and attached the delta patch as 0002. I've listed the changes in the commit message. Let me know if anything seems amiss.
Thanks Amit for your changes.
> * Revert (IMO) unnecessary modifications of build_child_join_sjinfo(). > For example, hunks to rename sjinfo to child_sjinfo seem unnecessary, > because it's clear from the context that they are "child" sjinfos. >
Ok.
> * Rename free_child_sjinfo_members() to free_child_join_sjinfo() to > be symmetric with build_child_join_sjinfo(). Note that the function > is charged with freeing the sjinfo itself too. Like > build_child_join_sjinfo(), name the argument sjinfo, not > child_sjinfo.
Yes. It should have been done in my patch.
> > * Don't set the child sjinfo pointer to NULL after freeing it. While > a good convention in general, it's not really common in the planner > code, so doing it here seems a bit overkill
That function is the last line in the block where pointer variable is declared. As of now there is no danger of the dangling pointer being used.
> > * Rename make_dummy_sjinfo() to init_dummy_sjinfo() because the > function is not really making it per se, only initializing fields > in the SpecialJoinInfo struct made available by the caller.
Right.
> > * Make init_dummy_sjinfo() take Relids instead of RelOptInfos because > that's what's needed. Also allows to reduce changes to > build_child_join_sjinfo()'s signature.
I remember, in one of the versions it was Relids and then I changed to RelOptInfos for some reason. The latest version seems to use just Relids. So this looks better.
> > * Update the reason mentioned in a comment in free_child_join_sjinfo() > about why semi_rhs_expr is not freed -- it may be freed, but there's > no way to "deep" free it, so we leave it alone. > > * Update a comment somewhere about why SpecialJoinInfo can be > "transient" sometimes.
With the later code, semi_rhs_expr is indeed free'able. It wasn't so when I wrote the patches. Thanks for updating the comment. I wish we would have freeObject(). Nothing that can be done for this patch.
Attached patches
Squashed your 0001 and 0002 into 0001. I have also reworded the commit message to reflect the latest code changes.
0002 has some minor edits. Please feel free to include or reject when committing the patch.
