[ back from PGCon ... ]
"Anton A. Melnikov" <aamelnikov@inbox.ru> writes:
> On 04.05.2023 15:22, Tom Lane wrote:
>> Under what circumstances would you be trying to inject INDEX_VAR
>> into a nullingrel set? Only outer-join relids should ever appear there.
> The thing is that i don't try to push INDEX_VAR into a nullingrel set at all,
> i just try to replace the existing rt_index that equals to INDEX_VAR in Var nodes with
> the defined positive indexes by using ChangeVarNodes_walker() function call.
Hmm. That implies that you're changing plan data structures around after
setrefs.c, which doesn't seem like a great design to me --- IMO that ought
to happen in PlanCustomPath, which will still see the original varnos.
However, it's probably not worth breaking existing code for this, so
now I agree that ChangeVarNodes ought to (continue to) allow negative
rt_index.
> Therefore it also seems better and more logical to me in the case of an index that
> cannot possibly be a member of the Bitmapset, immediately return false.
> Here is a patch like that.
I do not like the blast radius of this patch. Yes, I know about that
comment in bms_is_member --- I wrote it, if memory serves. But it's
stood like that for more than two decades, and I believe it's caught
its share of mistakes. This issue doesn't seem like a sufficient
reason to change a globally-visible behavior.
I think the right thing here is not either of your patches, but
to tweak adjust_relid_set() to not fail on negative oldrelid.
I'll go make it so.
regards, tom lane