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

On Thu, Feb 13, 2025 at 8:44 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
> 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.

When the translations happen, we do allocate memory for child bitmaps
so allocating and freeing memory is expected in the approach. However,
we can't blatantly copy the Relids returned by adjust_child_relids()
or we will leak the memory allocated if child Relids is a translation
of parent Relids. If the Relids output by adjust_child_relids() is not
the same as its input, we should copy bitmapset.

>
> 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.

This approach doesn't allocate memory when translation is not required
but it requires parent SJinfo to be present when
free_child_join_sjinfo() is called. It's true right now but future
changes to the code may not ensure that. So there is a potential
maintenance overhead here.

We have to be always cautious of freeing or modifying the Relids
returned by adjust_child_relids() [1]. For example, we could free
intermediate Relids in adjust_child_relids_multilevel() but we need to
be cautious of them being same as the parent Relids. Amit's approach
of bms_copy() in adjust_child_relids() frees us from that caution. May
be we could pass a flag to adjust_child_relids() asking it to create a
new bitmapset always. So even if it's a larger bandaid, it may be
useful at multiple places.

--
Best Wishes,
Ashutosh Bapat