Re: Memory consumed by child SpecialJoinInfo in partitionwise join planning

Hi Ashutosh,

On Thu, Sep 21, 2023 at 1:20 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> On Thu, Sep 21, 2023 at 6:37 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Wed, Sep 20, 2023 at 10:24 PM Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > > On Wed, Sep 20, 2023 at 5:24 PM Amit Langote <amitlangote09@gmail.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