Re: Memory consumed by child SpecialJoinInfo in partitionwise join planning

Hi Ashutosh,

On Thu, Sep 21, 2023 at 1:20 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> On Thu, Sep 21, 2023 at 6:37 AM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > On Wed, Sep 20, 2023 at 10:24 PM Ashutosh Bapat
> > <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> > > On Wed, Sep 20, 2023 at 5:24 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > > > Just one comment on 0003:
> > > >
> > > > + /*
> > > > + * Dummy SpecialJoinInfos do not have any translated fields and hence have
> > > > + * nothing to free.
> > > > + */
> > > > + if (child_sjinfo->jointype == JOIN_INNER)
> > > > + return;
> > > >
> > > > Should this instead be Assert(child_sjinfo->jointype != JOIN_INNER)?
> > >
> > > try_partitionwise_join() calls free_child_sjinfo_members()
> > > unconditionally i.e. it will be called even SpecialJoinInfos of INNER
> > > join. Above condition filters those out early. In fact if we convert
> > > into an Assert, in a production build the rest of the code will free
> > > Relids which are referenced somewhere else causing dangling pointers.
> >
> > You're right. I hadn't realized that the parent SpecialJoinInfo
> > passed to try_partitionwise_join() might itself be a dummy. I guess I
> > should've read the whole thing before commenting.
>
> Maybe there's something to improve in terms of readability, I don't know.

Here are some comments.

Please merge 0003 into 0002.

+ /*
+ * But the list of operator OIDs and the list of expressions may be
+ * referenced somewhere else. Do not free those.
+ */

I don't see build_child_join_sjinfo() copying any list of OIDs, so I'm
not sure what the comment is referring to. Also, instead of "the list
of expressions", it might be more useful to mention semi_rhs_expr,
because that's the only one being copied.

The comment for SpecialJoinInfo mentions that they are stored in
PlannerInfo.join_info_list, but the child SpecialJoinInfos used by
partitionwise joins are not, which I noticed has not been mentioned
anywhere. Should we make a note of that in the SpecialJoinInfo's
comment?

Just out of curiosity, is their not being present in join_info_list
problematic in some manner, such as missed optimization opportunities
for child joins? I noticed there is a loop over join_info_list in
add_paths_to_join_rel(), which does get called for child joinrels. I
know this a bit off-topic for the thread, but thought to ask in case
you've already thought about it.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com