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