database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-10-19 15:49:24 +03:00
Code Activity

Fix thinko in outer-join removal.

Browse Source 
If we have a RestrictInfo that mentions both the removal-candidate
relation and the outer join's relid, then that is a pushed-down
condition not a join condition, so it should be grounds for deciding
that we can't remove the outer join.  In commit 2489d76c4, I'd blindly
included the OJ's relid into "joinrelids" as per the new standard
convention, but the checks of attr_needed and ph_needed should only
allow the join's input rels to be mentioned.

Having done that, the check for references in pushed-down quals
a few lines further down should be redundant.  I left it in place
as an Assert, though.

While researching this I happened across a couple of comments that
worried about the effects of update_placeholder_eval_levels.
That's gone as of b448f1c8d, so we can remove some worry.

Per bug #17769 from Robins Tharakan.  The submitted test case
triggers this more or less accidentally because we flatten out
a LATERAL sub-select after we've done join strength reduction;
if we did that in the other order, this problem would be masked
because the outer join would get simplified to an inner join.
To ensure that the committed test case will continue to test
what it means to even if we make that happen someday, use a
test clause involving COALESCE(), which will prevent us from
using it to do join strength reduction.

Patch by me, but thanks to Richard Guo for initial investigation.

Discussion: https://postgr.es/m/17769-e4f7a5c9d84a80a7@postgresql.org
This commit is contained in:
Tom Lane
2023-02-04 15:19:54 -05:00
parent 5840c20272
commit 8538519db1
5 changed files with 39 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

30
src/backend/optimizer/plan/analyzejoins.c
View File

@@ -164,6 +164,7 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
{
int innerrelid;
RelOptInfo *innerrel;
Relids inputrelids;
Relids joinrelids;
List *clause_list = NIL;
ListCell *l;
@@ -190,17 +191,16 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
return false;
/* Compute the relid set for the join we are considering */
joinrelids = bms_union(sjinfo->min_lefthand, sjinfo->min_righthand);
if (sjinfo->ojrelid != 0)
joinrelids = bms_add_member(joinrelids, sjinfo->ojrelid);
inputrelids = bms_union(sjinfo->min_lefthand, sjinfo->min_righthand);
Assert(sjinfo->ojrelid != 0);
joinrelids = bms_copy(inputrelids);
joinrelids = bms_add_member(joinrelids, sjinfo->ojrelid);
/*
* We can't remove the join if any inner-rel attributes are used above the
* join.
*
* Note that this test only detects use of inner-rel attributes in higher
* join conditions and the target list. There might be such attributes in
* pushed-down conditions at this join, too. We check that case below.
* join. Here, "above" the join includes pushed-down conditions, so we
* should reject if attr_needed includes the OJ's own relid; therefore,
* compare to inputrelids not joinrelids.
*
* As a micro-optimization, it seems better to start with max_attr and
* count down rather than starting with min_attr and counting up, on the
@@ -211,7 +211,7 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
attroff >= 0;
attroff--)
{
if (!bms_is_subset(innerrel->attr_needed[attroff], joinrelids))
if (!bms_is_subset(innerrel->attr_needed[attroff], inputrelids))
return false;
}
@@ -230,7 +230,7 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
if (bms_overlap(phinfo->ph_lateral, innerrel->relids))
return false; /* it references innerrel laterally */
if (bms_is_subset(phinfo->ph_needed, joinrelids))
if (bms_is_subset(phinfo->ph_needed, inputrelids))
continue; /* PHV is not used above the join */
if (!bms_overlap(phinfo->ph_eval_at, innerrel->relids))
continue; /* it definitely doesn't reference innerrel */
@@ -273,13 +273,11 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
{
/*
* If such a clause actually references the inner rel then join
* removal has to be disallowed. We have to check this despite
* the previous attr_needed checks because of the possibility of
* pushed-down clauses referencing the rel.
* removal has to be disallowed. The previous attr_needed checks
* should have caught this, though.
*/
if (bms_is_member(innerrelid, restrictinfo->clause_relids))
return false;
continue; /* else, ignore; not useful here */
Assert(!bms_is_member(innerrelid, restrictinfo->clause_relids));
continue; /* ignore; not useful here */
}
/* Ignore if it's not a mergejoinable clause */