Re: BUG #18806: When enable_rartitionwise_join is set to ON, the database shuts down abnormally

On Thu, Feb 13, 2025 at 10:39 AM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Wed, Feb 12, 2025 at 11:30 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> > On Wed, Feb 12, 2025 at 10:04 PM Ashutosh Bapat
> > <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> > > Hmm. The code there assumes that all the Relids will at least have one
> > > parent each of the children involved. For some reason
> > > sjinfo->min_lefthand has only relid 1 but not 2 or 5. 2 and 5 are
> > > actually the parent relids of the children passed in respectively. The
> > > join is between 2 and 5, then why is 1 appearing in the min_lefthand.
> > > It might be legitimate, but we need to find the reason. If it's
> > > legitimate, I think we need to copy the Relids which haven't undergone
> > > any translation so as to keep them isolated from the parent relids.
> >
> > Yes, it's legitimate. For the semijoin, its join clause only
> > references {1} and {5}, with no other ordering restrictions.
> > Therefore, the minimum LHS for this join consists only of {1}.
> >
> > Instead of copying the untranslated Relids and freeing them later, I
> > think it might be better to modify free_child_join_sjinfo() to avoid
> > freeing the untranslated members of child_sjinfo.
>
> free_child_join_sjinfo() frees min_lefthand and other fields
> initialized by build_child_join_sjinfo(), assuming that
> adjust_child_relids() creates a copy. However, this does not always
> seem to be the case, as demonstrated in this report.
>
> I'm wondering if the following line in adjust_child_relids() should be
> using bms_copy() instead:
>
> /* Otherwise, return the original set without modification. */
> return relids;
> }
>
> That is, should we copy relids not only when translation is needed but
> also in the general case? Would that be a bigger band-aid than
> necessary?

Hmm, I'm concerned this might lead to unnecessary memory usage in
other translation scenarios. After all, the purpose of 5278d0a2e is
to reduce memory used by partitionwise joins. Even in the
SpecialJoinInfo case, this approach means that we are copying
untranslated Relids and then freeing them later, which seems
unnecessary.

I'm thinking that a better approach might be to check if each member
of the child_sjinfo is a translated copy and only free it if that's
the case, something like attached.

Thanks
Richard