Re: BUG #18260: Unexpected error: "negative bitmapset member not allowed" triggered by multiple JOIN

On Mon, Jan 15, 2024 at 2:20 AM Andrei Lepikhov
<a.lepikhov@postgrespro.ru> wrote:
> On 11/1/2024 16:08, Richard Guo wrote:
> >
> > On Thu, Dec 28, 2023 at 12:30 PM Andrei Lepikhov
> > <a.lepikhov@postgrespro.ru <mailto:a.lepikhov@postgrespro.ru>> wrote:
> >
> >     On 28/12/2023 10:03, Richard Guo wrote:
> >      >          if (bms_is_subset(phinfo->ph_needed, joinrelids) &&
> >      >              bms_is_member(relid, phinfo->ph_eval_at) &&
> >      > -           !bms_is_member(ojrelid, phinfo->ph_eval_at))
> >      > +           (sjinfo == NULL || !bms_is_member(ojrelid,
> >     phinfo->ph_eval_at)))
> >
> >      > Alternatively, we can modify bms_is_member() to return false for
> >      > negative numbers instead of emitting an error, as suggested by the
> >      > comment there.
> >      > -   /* XXX better to just return false for x<0 ? */
> >      > +   /* negative number cannot be a member of the bitmapset */
> >      >      if (x < 0)
> >      > -       elog(ERROR, "negative bitmapset member not allowed");
> >      > +       return false;
> >      >
> >      > I prefer the second option, but I'm open to other thoughts.
> >     I don't like this option. It could mask some blunders somewhere like a
> >     current one.
> >
> >
> > After a second thought, I agree that the first option is better.  It
> > maintains consistency with the Assert a few lines ahead, and also
> > minimizes the potential impact of code changes compared to the second
> > option.
> >
> >      > FWIW, here is a simplified repro for this error.
> >      >
> >      > create table t (a int primary key, b int);
> >      >
> >      > explain (verbose, costs off)
> >      > select 1 from t t1 left join
> >      >      (lateral (select 1 as x, * from t t2) s1 inner join
> >      >          (select * from t t3) s2 on s1.a = s2.a)
> >      >      on true
> >      > where s1.x = 1;
> >      > ERROR:  negative bitmapset member not allowed
> >     It can be simplified even more (without LATERAL):
> >
> >
> > Indeed.  I realized this shortly after v1 patch was sent out, and was
> > thinking that it wasn't worth the effort to update the patch with a new
> > version just for this issue.
> I like it, thanks for your efforts.

+1,
Thank you, Richard, Andrei.  I'm going to push this tomorrow.

------
Regards,
Alexander Korotkov