Horton

Re: Making Vars outer-join aware

Tom Lane <tgl@sss.pgh.pa.us>

View thread
From: Tom Lane <tgl@sss.pgh.pa.us>
To: "Anton A. Melnikov" <aamelnikov@inbox.ru>
Cc: Justin Pryzby <pryzby@telsasoft.com>, Richard Guo <guofenglinux@gmail.com>, pgsql-hackers@lists.postgresql.org, "Finnerty, Jim" <jfinnert@amazon.com>
Date: 2023-06-08T16:58:08Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Re-allow INDEX_VAR as rt_index in ChangeVarNodes().

  2. Fix thinkos in have_unsafe_outer_join_ref; reduce to Assert check.

  3. Invent "join domains" to replace the below_outer_join hack.

  4. Do assorted mop-up in the planner.

  5. Make Vars be outer-join-aware.

  6. Invent "multibitmapsets", and use them to speed up antijoin detection.

  7. Add basic regression tests for semi/antijoin recognition.

  8. Improve performance of adjust_appendrel_attrs_multilevel.

  9. Refactor addition of PlaceHolderVars to joinrel targetlists.

  10. Use an explicit state flag to control PlaceHolderInfo creation.

  11. Make PlaceHolderInfo lookup O(1).

[ 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