Re: Memory consumed by child SpecialJoinInfo in partitionwise join planning

On Fri, Mar 22, 2024 at 2:54 PM Amit Langote <amitlangote09(at)gmail(dot)com>
wrote:

> >
> > Please let me know if you need anything.
>
> 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.

--
Best Wishes,
Ashutosh Bapat