Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 8538519

Browse filesBrowse files
tglsfdctglsfdc
committed
Fix thinko in outer-join removal.
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 2489d76, 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 b448f1c, 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
1 parent 5840c20 commit 8538519
Copy full SHA for 8538519

File tree

5 files changed

+39
-29
lines changed
Filter options

5 files changed

+39
-29
lines changed

‎src/backend/optimizer/plan/analyzejoins.c

Copy file name to clipboardExpand all lines: src/backend/optimizer/plan/analyzejoins.c
+14-16Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,7 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
164164
{
165165
int innerrelid;
166166
RelOptInfo *innerrel;
167+
Relids inputrelids;
167168
Relids joinrelids;
168169
List *clause_list = NIL;
169170
ListCell *l;
@@ -190,17 +191,16 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
190191
return false;
191192

192193
/* Compute the relid set for the join we are considering */
193-
joinrelids = bms_union(sjinfo->min_lefthand, sjinfo->min_righthand);
194-
if (sjinfo->ojrelid != 0)
195-
joinrelids = bms_add_member(joinrelids, sjinfo->ojrelid);
194+
inputrelids = bms_union(sjinfo->min_lefthand, sjinfo->min_righthand);
195+
Assert(sjinfo->ojrelid != 0);
196+
joinrelids = bms_copy(inputrelids);
197+
joinrelids = bms_add_member(joinrelids, sjinfo->ojrelid);
196198

197199
/*
198200
* We can't remove the join if any inner-rel attributes are used above the
199-
* join.
200-
*
201-
* Note that this test only detects use of inner-rel attributes in higher
202-
* join conditions and the target list. There might be such attributes in
203-
* pushed-down conditions at this join, too. We check that case below.
201+
* join. Here, "above" the join includes pushed-down conditions, so we
202+
* should reject if attr_needed includes the OJ's own relid; therefore,
203+
* compare to inputrelids not joinrelids.
204204
*
205205
* As a micro-optimization, it seems better to start with max_attr and
206206
* count down rather than starting with min_attr and counting up, on the
@@ -211,7 +211,7 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
211211
attroff >= 0;
212212
attroff--)
213213
{
214-
if (!bms_is_subset(innerrel->attr_needed[attroff], joinrelids))
214+
if (!bms_is_subset(innerrel->attr_needed[attroff], inputrelids))
215215
return false;
216216
}
217217

@@ -230,7 +230,7 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
230230

231231
if (bms_overlap(phinfo->ph_lateral, innerrel->relids))
232232
return false; /* it references innerrel laterally */
233-
if (bms_is_subset(phinfo->ph_needed, joinrelids))
233+
if (bms_is_subset(phinfo->ph_needed, inputrelids))
234234
continue; /* PHV is not used above the join */
235235
if (!bms_overlap(phinfo->ph_eval_at, innerrel->relids))
236236
continue; /* it definitely doesn't reference innerrel */
@@ -273,13 +273,11 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
273273
{
274274
/*
275275
* If such a clause actually references the inner rel then join
276-
* removal has to be disallowed. We have to check this despite
277-
* the previous attr_needed checks because of the possibility of
278-
* pushed-down clauses referencing the rel.
276+
* removal has to be disallowed. The previous attr_needed checks
277+
* should have caught this, though.
279278
*/
280-
if (bms_is_member(innerrelid, restrictinfo->clause_relids))
281-
return false;
282-
continue; /* else, ignore; not useful here */
279+
Assert(!bms_is_member(innerrelid, restrictinfo->clause_relids));
280+
continue; /* ignore; not useful here */
283281
}
284282

285283
/* Ignore if it's not a mergejoinable clause */

‎src/backend/optimizer/plan/initsplan.c

Copy file name to clipboardExpand all lines: src/backend/optimizer/plan/initsplan.c
+3-3Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -494,9 +494,6 @@ extract_lateral_references(PlannerInfo *root, RelOptInfo *brel, Index rtindex)
494494
* create_lateral_join_info
495495
* Fill in the per-base-relation direct_lateral_relids, lateral_relids
496496
* and lateral_referencers sets.
497-
*
498-
* This has to run after deconstruct_jointree, because we need to know the
499-
* final ph_eval_at values for PlaceHolderVars.
500497
*/
501498
void
502499
create_lateral_join_info(PlannerInfo *root)
@@ -509,6 +506,9 @@ create_lateral_join_info(PlannerInfo *root)
509506
if (!root->hasLateralRTEs)
510507
return;
511508

509+
/* We'll need to have the ph_eval_at values for PlaceHolderVars */
510+
Assert(root->placeholdersFrozen);
511+
512512
/*
513513
* Examine all baserels (the rel array has been set up by now).
514514
*/

‎src/backend/optimizer/util/var.c

Copy file name to clipboardExpand all lines: src/backend/optimizer/util/var.c
-10Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -198,16 +198,6 @@ pull_varnos_walker(Node *node, pull_varnos_context *context)
198198
* fall back to the conservative assumption that the PHV will be
199199
* evaluated at its syntactic level (phv->phrels).
200200
*
201-
* There is a second hazard: this code is also used to examine
202-
* qual clauses during deconstruct_jointree, when we may have a
203-
* PlaceHolderInfo but its ph_eval_at value is not yet final, so
204-
* that theoretically we could obtain a relid set that's smaller
205-
* than we'd see later on. That should never happen though,
206-
* because we deconstruct the jointree working upwards. Any outer
207-
* join that forces delay of evaluation of a given qual clause
208-
* will be processed before we examine that clause here, so the
209-
* ph_eval_at value should have been updated to include it.
210-
*
211201
* Another problem is that a PlaceHolderVar can appear in quals or
212202
* tlists that have been translated for use in a child appendrel.
213203
* Typically such a PHV is a parameter expression sourced by some

‎src/test/regress/expected/join.out

Copy file name to clipboardExpand all lines: src/test/regress/expected/join.out
+15Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5150,6 +5150,21 @@ SELECT * FROM
51505150
1 | 4567890123456789 | -4567890123456789 | 4567890123456789
51515151
(5 rows)
51525152

5153+
-- join removal bug #17769: can't remove if there's a pushed-down reference
5154+
EXPLAIN (COSTS OFF)
5155+
SELECT q2 FROM
5156+
(SELECT *
5157+
FROM int8_tbl LEFT JOIN innertab ON q2 = id) ss
5158+
WHERE COALESCE(dat1, 0) = q1;
5159+
QUERY PLAN
5160+
----------------------------------------------------------------
5161+
Nested Loop Left Join
5162+
Filter: (COALESCE(innertab.dat1, '0'::bigint) = int8_tbl.q1)
5163+
-> Seq Scan on int8_tbl
5164+
-> Index Scan using innertab_pkey on innertab
5165+
Index Cond: (id = int8_tbl.q2)
5166+
(5 rows)
5167+
51535168
rollback;
51545169
-- another join removal bug: we must clean up correctly when removing a PHV
51555170
begin;

‎src/test/regress/sql/join.sql

Copy file name to clipboardExpand all lines: src/test/regress/sql/join.sql
+7Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1860,6 +1860,13 @@ SELECT * FROM
18601860
FROM int8_tbl LEFT JOIN innertab ON q2 = id) ss2
18611861
ON true;
18621862

1863+
-- join removal bug #17769: can't remove if there's a pushed-down reference
1864+
EXPLAIN (COSTS OFF)
1865+
SELECT q2 FROM
1866+
(SELECT *
1867+
FROM int8_tbl LEFT JOIN innertab ON q2 = id) ss
1868+
WHERE COALESCE(dat1, 0) = q1;
1869+
18631870
rollback;
18641871

18651872
-- another join removal bug: we must clean up correctly when removing a PHV

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.