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.
