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. -- Best Wishes, Ashutosh Bapat
