Re: Clause accidentally pushed down ( Possible bug in Making Vars outer-join aware)

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Richard Guo <guofenglinux(at)gmail(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Andrey Lepikhov <a(dot)lepikhov(at)postgrespro(dot)ru>, PostgreSQL mailing lists <pgsql-bugs(at)lists(dot)postgresql(dot)org>
Subject: Re: Clause accidentally pushed down ( Possible bug in Making Vars outer-join aware)
Date: 2023-05-17 03:10:45
Message-ID: 3014965.1684293045@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs

After further poking at this, I've concluded that you're right and
we should not try to make the "add extra nullingrels all the time"
approach work. It still seems theoretically cleaner to me, but
there are too many messy special cases.

Accordingly, here's a reviewed/revised version of the v3 patch series
you posted earlier. Significant changes:

* I was not impressed by the idea of adding a join_info_array to look
up by ojrelid. It seemed to me that your changes around that replaced
foreach loops over join_info_list with bms_next_member loops over
relid sets that you often had to do extra calculations to create.
Whether that's a performance win at all is debatable. Moreover,
I realized from testing this that situations where there are
pushed-down OJs are pretty rare (there are *none* in the core
regression tests before the cases added below), so optimizing for
that at the cost of extra cycles when it doesn't happen is probably
not the way.

* I did keep the idea of splitting up commute_below from my other
patch series. That's 0001 below and then I rebased 0002-0004 on
that.

* I'm not terribly happy with 0004 as it stands, specifically the
business of making build_join_rel recalculate pushed_down_ojrelids
from scratch. Again, that's adding cycles to the mainline case to
support an unusual case. It'd be better to make
add_outer_joins_to_relids have an extra output that is the
pushed_down_ojrelids set, and pass that forward. I ran out of time
to make that happen today, though.

* 0005 below adds the original test case and cases based on the other
examples given at various points in this thread. I put that last
because until 0004 one or the other of these tests fails miserably.
All the expected outputs here match what v15 does.

* While I've not done it here, I'm seriously considering reverting
those Asserts in setrefs.c back to somewhat-informative elogs,
at least till late in beta. It seems like we are still at a point
where we need to make it easy to acquire info about failures in this
logic, and using Asserts isn't all that conducive to that.

regards, tom lane

Attachment Content-Type Size
v6-0001-Split-SpecialJoinInfo.commute_below-into-LHS-and-.patch text/x-diff 10.0 KB
v6-0002-Postpone-adding-pushed-down-ojrelid-to-a-join-s-r.patch text/x-diff 14.9 KB
v6-0003-Examine-the-transitive-closure-in-add_outer_joins.patch text/x-diff 3.1 KB
v6-0004-Fix-additions-of-nullingrels-to-joinrels-output-t.patch text/x-diff 6.3 KB
v6-0005-Add-test-cases-for-new-join-planning-logic.patch text/x-diff 7.4 KB

In response to

Responses

Browse pgsql-bugs by date

  From Date Subject
Next Message Michael Paquier 2023-05-17 05:23:40 Re: BUG #17884: gist_page_items() crashes for a non-leaf page of an index with non-key columns
Previous Message Dave Cramer 2023-05-16 22:28:42 Re: BUG #17911: Database or JDBC Driver Provides Incorrect Type