From c7265746d77679a37a36a0658d7f55cc2583dadf Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 12 May 2023 12:52:53 -0400 Subject: [PATCH v4 2/2] Fix mishandling of quals above a commuted pair of outer joins. If the planner applied outer join identity 3 in the forward direction, it was possible for higher qual clauses referencing the "C" relation to drop down to the B/C join (below the now-upper A/B join). This gave the wrong answers, since values that would become NULL after applying the A/B join might not be NULL at that point. There is no hazard if the original input is written in the second form of the identity, because then upper C Vars will be marked as nullable by both joins so we'll make correct qual placement decisions whether we commute the joins or not. Hence, the best fix seems to be to forcibly mark upper C Vars as nullable by both joins once we detect that they are potentially commutable. Per report from Andrey Lepikhov. Thanks to Richard Guo for investigation. Discussion: https://postgr.es/m/0b819232-4b50-f245-1c7d-c8c61bf41827@postgrespro.ru --- src/backend/optimizer/README | 16 ++ src/backend/optimizer/plan/initsplan.c | 241 ++++++++++++++++++++++++- src/backend/optimizer/util/relnode.c | 58 +++--- src/backend/rewrite/rewriteManip.c | 9 +- src/test/regress/expected/join.out | 34 ++++ src/test/regress/sql/join.sql | 16 ++ 6 files changed, 339 insertions(+), 35 deletions(-) diff --git a/src/backend/optimizer/README b/src/backend/optimizer/README index 227278eb6c..f41b368c1c 100644 --- a/src/backend/optimizer/README +++ b/src/backend/optimizer/README @@ -507,6 +507,22 @@ problem for join relation identification either, since whether a semijoin has been completed is again implicit in the set of base relations included in the join. +As usual, outer join identity 3 complicates matters. If we start with + (A leftjoin B on (Pab)) leftjoin C on (Pbc) +then the parser will have marked any C Vars appearing above these joins +with the RT index of the B/C join. If we now transform to + A leftjoin (B leftjoin C on (Pbc)) on (Pab) +then it would appear that a clause using only such Vars could be pushed +down and applied as a filter clause (not a join clause) at the lower B/C +join. But *this might not give the right answer* since the clause might +see a non-null value for the C Var that will be replaced by null once +the A/B join is performed. There is no danger if the original input was +in the second form, since then upper C Vars will be marked with the RT +indexes of both joins, and the normal qual placement rules will ensure +that quals are placed appropriately. We handle this problem by forcibly +marking upper C Vars with both RT indexes when we detect that we have a +commutable pair of joins that were initially written in the first form. + There is one additional complication for qual clause placement, which occurs when we have made multiple versions of an outer-join clause as described previously (that is, we have both "Pbc" and "Pb*c" forms of diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index 5cbb7b9a86..9d72437c6a 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -31,6 +31,7 @@ #include "optimizer/prep.h" #include "optimizer/restrictinfo.h" #include "parser/analyze.h" +#include "parser/parsetree.h" #include "rewrite/rewriteManip.h" #include "utils/lsyscache.h" #include "utils/typcache.h" @@ -97,6 +98,12 @@ static SpecialJoinInfo *make_outerjoininfo(PlannerInfo *root, Relids inner_join_rels, JoinType jointype, Index ojrelid, List *clause); +static void update_commutable_vars(PlannerInfo *root, SpecialJoinInfo *sjinfo, + SpecialJoinInfo *otherinfo); +static void update_commutable_vars_in_jointree(PlannerInfo *root, + Node *jtnode, + SpecialJoinInfo *sjinfo, + Bitmapset *added_relids); static void compute_semijoin_info(PlannerInfo *root, SpecialJoinInfo *sjinfo, List *clause); static void deconstruct_distribute_oj_quals(PlannerInfo *root, @@ -1655,7 +1662,10 @@ make_outerjoininfo(PlannerInfo *root, /* Anything left? */ if (commute_below_l || commute_below_r) { - /* Yup, so we must update the derived data in the SpecialJoinInfos */ + /* + * Yup, so we must update the derived data in the SpecialJoinInfos, + * and adjust upper Vars if needed. + */ sjinfo->commute_below_l = commute_below_l; sjinfo->commute_below_r = commute_below_r; foreach(l, root->join_info_list) @@ -1663,17 +1673,246 @@ make_outerjoininfo(PlannerInfo *root, SpecialJoinInfo *otherinfo = (SpecialJoinInfo *) lfirst(l); if (bms_is_member(otherinfo->ojrelid, commute_below_l)) + { otherinfo->commute_above_l = bms_add_member(otherinfo->commute_above_l, ojrelid); + update_commutable_vars(root, sjinfo, otherinfo); + } else if (bms_is_member(otherinfo->ojrelid, commute_below_r)) + { otherinfo->commute_above_r = bms_add_member(otherinfo->commute_above_r, ojrelid); + /* no need to change upper Vars in this case */ + } } } return sjinfo; } +/* + * update_commutable_vars + * Mark Vars above a commutable pair of outer joins with both OJs' relids + * + * We have identified a pair of outer joins that are commutable according to + * outer join identity 3, and were written in the first form of the identity: + * (A leftjoin B on (Pab)) leftjoin C on (Pbc) + * This means that any C Vars appearing above the two joins are marked with + * only the B/C join's relid in varnullingrels. Had the query been written + * according to the second form, + * A leftjoin (B leftjoin C on (Pbc)) on (Pab) + * then upper C vars would have both OJs' relids in their varnullingrels. + * + * Our task here is to mark such upper C Vars (and PlaceHolderVars) with + * the A/B join's relid too, so that they will look the same regardless of + * the query's syntactic form. One critical reason for doing this is that + * if we do commute to the second form, quals using these Vars must not be + * allowed to drop below the A/B join: the Vars might not yet be null in + * join rows where they should be null. + * + * Because this is invoked during a recursive scan of the jointree, we cannot + * mutate the jointree's structure (compare perform_pullup_replace_vars() in + * prepjointree.c, which has a similar constraint). So we can't just let + * add_nulling_relids() loose on the whole tree; we have to be careful to + * apply it only to the qual subtrees. We can save a little bit of work + * by not descending into jointree sections we've already processed, since + * those could not contain references to the Vars of interest. + * + * sjinfo: just-completed SpecialJoinInfo for the syntactically-upper B/C join + * otherinfo: SpecialJoinInfo for the syntactically-lower A/B join + * sjinfo is not yet part of root->join_info_list, but otherinfo is + */ +static void +update_commutable_vars(PlannerInfo *root, SpecialJoinInfo *sjinfo, + SpecialJoinInfo *otherinfo) +{ + Query *parse = root->parse; + Bitmapset *target_relids; + Bitmapset *added_relids; + ListCell *lc; + + /* + * We can use add_nulling_relids() to do the grunt work within + * subexpressions. Tell it to add otherinfo->ojrelid to nullingrels of + * anything referencing the upper join's min RHS. + */ + target_relids = sjinfo->min_righthand; + Assert(otherinfo->ojrelid != 0); + added_relids = bms_make_singleton(otherinfo->ojrelid); + + /* + * Update Vars appearing in the targetList, returningList, and other + * top-level structures. + */ + parse->targetList = (List *) + add_nulling_relids((Node *) parse->targetList, + target_relids, added_relids); + parse->returningList = (List *) + add_nulling_relids((Node *) parse->returningList, + target_relids, added_relids); + root->processed_tlist = (List *) + add_nulling_relids((Node *) root->processed_tlist, + target_relids, added_relids); + + foreach(lc, parse->windowClause) + { + WindowClause *wc = lfirst_node(WindowClause, lc); + + if (wc->runCondition != NIL) + wc->runCondition = (List *) + add_nulling_relids((Node *) wc->runCondition, + target_relids, added_relids); + } + if (parse->onConflict) + { + parse->onConflict->onConflictSet = (List *) + add_nulling_relids((Node *) parse->onConflict->onConflictSet, + target_relids, added_relids); + parse->onConflict->onConflictWhere = + add_nulling_relids(parse->onConflict->onConflictWhere, + target_relids, added_relids); + + /* + * We assume ON CONFLICT's arbiterElems, arbiterWhere, exclRelTlist + * can't contain any references to an outer join's result. + */ + } + if (parse->mergeActionList) + { + foreach(lc, parse->mergeActionList) + { + MergeAction *action = lfirst(lc); + + action->qual = add_nulling_relids(action->qual, + target_relids, added_relids); + action->targetList = (List *) + add_nulling_relids((Node *) action->targetList, + target_relids, added_relids); + } + } + update_commutable_vars_in_jointree(root, (Node *) parse->jointree, + sjinfo, added_relids); + Assert(parse->setOperations == NULL); + parse->havingQual = add_nulling_relids(parse->havingQual, + target_relids, added_relids); + + /* + * Unlike perform_pullup_replace_vars(), we needn't process appendrels' + * translated_vars lists, because there won't be any join output vars + * there. We needn't process joinaliasvars lists either --- those are + * empty at this point, cf subquery_planner(). + */ +} + +/* + * Helper routine for update_commutable_vars: do add_nulling_relids on every + * expression in the jointree, without changing the jointree structure itself. + * Ugly, but there's no other way... + */ +static void +update_commutable_vars_in_jointree(PlannerInfo *root, + Node *jtnode, + SpecialJoinInfo *sjinfo, + Bitmapset *added_relids) +{ + Bitmapset *target_relids = sjinfo->min_righthand; + + if (jtnode == NULL) + return; + if (IsA(jtnode, RangeTblRef)) + { + /* + * If the RangeTblRef refers to a LATERAL subquery, it might contain + * references to the target join, which we must update. We drive this + * from the jointree scan, rather than a scan of the rtable, so that + * we can avoid processing no-longer-referenced RTEs. + */ + int varno = ((RangeTblRef *) jtnode)->rtindex; + RangeTblEntry *rte = rt_fetch(varno, root->parse->rtable); + RelOptInfo *rel; + + if (rte->lateral) + { + /* Update sub-structure of the RTE */ + switch (rte->rtekind) + { + case RTE_RELATION: + /* shouldn't be marked LATERAL unless tablesample */ + Assert(rte->tablesample); + rte->tablesample = (TableSampleClause *) + add_nulling_relids((Node *) rte->tablesample, + target_relids, added_relids); + break; + case RTE_SUBQUERY: + /* this does the right thing, see add_nulling_relids */ + rte->subquery = (Query *) + add_nulling_relids((Node *) rte->subquery, + target_relids, added_relids); + break; + case RTE_FUNCTION: + rte->functions = (List *) + add_nulling_relids((Node *) rte->functions, + target_relids, added_relids); + break; + case RTE_TABLEFUNC: + rte->tablefunc = (TableFunc *) + add_nulling_relids((Node *) rte->tablefunc, + target_relids, added_relids); + break; + case RTE_VALUES: + rte->values_lists = (List *) + add_nulling_relids((Node *) rte->values_lists, + target_relids, added_relids); + break; + case RTE_JOIN: + case RTE_CTE: + case RTE_NAMEDTUPLESTORE: + case RTE_RESULT: + /* these shouldn't be marked LATERAL */ + Assert(false); + break; + } + + /* + * We must also update the rel's lateral_vars list, since that's + * already been constructed. + */ + rel = find_base_rel(root, varno); + rel->lateral_vars = (List *) + add_nulling_relids((Node *) rel->lateral_vars, + target_relids, added_relids); + } + } + else if (IsA(jtnode, FromExpr)) + { + FromExpr *f = (FromExpr *) jtnode; + ListCell *l; + + foreach(l, f->fromlist) + update_commutable_vars_in_jointree(root, lfirst(l), + sjinfo, added_relids); + f->quals = add_nulling_relids(f->quals, target_relids, added_relids); + } + else if (IsA(jtnode, JoinExpr)) + { + JoinExpr *j = (JoinExpr *) jtnode; + + /* + * We don't need to recurse into the children or quals of the upper + * commutable join. + */ + if (j->rtindex == sjinfo->ojrelid) + return; + + update_commutable_vars_in_jointree(root, j->larg, sjinfo, added_relids); + update_commutable_vars_in_jointree(root, j->rarg, sjinfo, added_relids); + j->quals = add_nulling_relids(j->quals, target_relids, added_relids); + } + else + elog(ERROR, "unrecognized node type: %d", + (int) nodeTag(jtnode)); +} + /* * compute_semijoin_info * Fill semijoin-related fields of a new SpecialJoinInfo diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c index 68fd033595..73c1457e07 100644 --- a/src/backend/optimizer/util/relnode.c +++ b/src/backend/optimizer/util/relnode.c @@ -1039,33 +1039,29 @@ min_join_parameterization(PlannerInfo *root, * of data that was cached at the baserel level by set_rel_width(). * * Pass can_null as true if the join is an outer join that can null Vars - * from this input relation. If so, we will (normally) add the join's relid + * from this input relation. If so, we will add the join's relid (if any) * to the nulling bitmaps of Vars and PHVs bubbled up from the input. * * When forming an outer join's target list, special handling is needed - * in case the outer join was commuted with another one per outer join - * identity 3 (see optimizer/README). We must take steps to ensure that - * the output Vars have the same nulling bitmaps that they would if the - * two joins had been done in syntactic order; else they won't match Vars - * appearing higher in the query tree. We need to do two things: - * - * First, we add the outer join's relid to the nulling bitmap only if the Var - * or PHV actually comes from within the syntactically nullable side(s) of the - * outer join. This takes care of the possibility that we have transformed - * (A leftjoin B on (Pab)) leftjoin C on (Pbc) - * to - * A leftjoin (B leftjoin C on (Pbc)) on (Pab) - * Here the now-upper A/B join must not mark C columns as nulled by itself. - * - * Second, any relid in sjinfo->commute_above_r that is already part of - * the joinrel is added to the nulling bitmaps of nullable Vars and PHVs. - * This takes care of the reverse case where we implement - * A leftjoin (B leftjoin C on (Pbc)) on (Pab) - * as + * in case the outer join can commute with another one per outer join + * identity 3 (see optimizer/README). We must ensure that the output Vars + * have nulling bitmaps matching what higher levels of the tree expect. + * We previously forced join-output Vars in the higher levels to have both + * joins' relids in their nulling bitmaps (see update_commutable_vars()). + * To match that outcome, any relid in sjinfo->commute_below_l that is already + * part of the joinrel is added to the nulling bitmaps of nullable Vars and + * PHVs. This takes care of the possibility that the join is written and + * implemented in the form * (A leftjoin B on (Pab)) leftjoin C on (Pbc) * The C columns emitted by the B/C join need to be shown as nulled by both - * the B/C and A/B joins, even though they've not physically traversed the - * A/B join. + * the B/C and A/B joins, even though they've not physically traversed the A/B + * join. Likewise, any relid in sjinfo->commute_above_r that is already part + * of the joinrel is added to the nulling bitmaps of nullable Vars and PHVs. + * This takes care of the case where we originally had + * A leftjoin (B leftjoin C on (Pbc)) on (Pab) + * but it's been transformed to the first form. (If we have implemented + * the joins in this second form, no additional work is needed, no matter + * which form was written originally.) */ static void build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel, @@ -1100,12 +1096,13 @@ build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel, { phv = copyObject(phv); /* See comments above to understand this logic */ - if (sjinfo->ojrelid != 0 && - (bms_is_subset(phv->phrels, sjinfo->syn_righthand) || - (sjinfo->jointype == JOIN_FULL && - bms_is_subset(phv->phrels, sjinfo->syn_lefthand)))) + if (sjinfo->ojrelid != 0) phv->phnullingrels = bms_add_member(phv->phnullingrels, sjinfo->ojrelid); + phv->phnullingrels = + bms_join(phv->phnullingrels, + bms_intersect(sjinfo->commute_below_l, + relids)); phv->phnullingrels = bms_join(phv->phnullingrels, bms_intersect(sjinfo->commute_above_r, @@ -1165,12 +1162,13 @@ build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel, { var = copyObject(var); /* See comments above to understand this logic */ - if (sjinfo->ojrelid != 0 && - (bms_is_member(var->varno, sjinfo->syn_righthand) || - (sjinfo->jointype == JOIN_FULL && - bms_is_member(var->varno, sjinfo->syn_lefthand)))) + if (sjinfo->ojrelid != 0) var->varnullingrels = bms_add_member(var->varnullingrels, sjinfo->ojrelid); + var->varnullingrels = + bms_join(var->varnullingrels, + bms_intersect(sjinfo->commute_below_l, + relids)); var->varnullingrels = bms_join(var->varnullingrels, bms_intersect(sjinfo->commute_above_r, diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c index 52b3f77078..76d4d13e47 100644 --- a/src/backend/rewrite/rewriteManip.c +++ b/src/backend/rewrite/rewriteManip.c @@ -1129,6 +1129,10 @@ AddInvertedQual(Query *parsetree, Node *qual) * add_nulling_relids() finds Vars and PlaceHolderVars that belong to any * of the target_relids, and adds added_relids to their varnullingrels * and phnullingrels fields. + * + * Note: unlike many of the other functions in this file, if this is invoked + * directly on a Query node, it will mutate contained Vars of level 1 not + * level 0. This is desirable for current uses. */ Node * add_nulling_relids(Node *node, @@ -1140,10 +1144,7 @@ add_nulling_relids(Node *node, context.target_relids = target_relids; context.added_relids = added_relids; context.sublevels_up = 0; - return query_or_expression_tree_mutator(node, - add_nulling_relids_mutator, - &context, - 0); + return add_nulling_relids_mutator(node, &context); } static Node * diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index b5f440e43e..39c57d5c59 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -2357,6 +2357,40 @@ where b.f1 = t.thousand and a.f1 = b.f1 and (a.f1+b.f1+999) = t.tenthous; ----+----+----------+---------- (0 rows) +-- +-- check for correct placement of non-strict qual in a multiway outer join +-- +explain (costs off) +select t1.f1 +from int4_tbl t1, int4_tbl t2 + left join int4_tbl t3 on t3.f1 > 0 + left join int4_tbl t4 on t3.f1 > 1 +where t4.f1 is null; + QUERY PLAN +------------------------------------------------------- + Nested Loop + -> Nested Loop Left Join + Filter: (t4.f1 IS NULL) + -> Seq Scan on int4_tbl t2 + -> Materialize + -> Nested Loop Left Join + Join Filter: (t3.f1 > 1) + -> Seq Scan on int4_tbl t3 + Filter: (f1 > 0) + -> Materialize + -> Seq Scan on int4_tbl t4 + -> Seq Scan on int4_tbl t1 +(12 rows) + +select t1.f1 +from int4_tbl t1, int4_tbl t2 + left join int4_tbl t3 on t3.f1 > 0 + left join int4_tbl t4 on t3.f1 > 1 +where t4.f1 is null; + f1 +---- +(0 rows) + -- -- check a case where we formerly got confused by conflicting sort orders -- in redundant merge join path keys diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 437934e80b..fc2ea35a4f 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -441,6 +441,22 @@ select a.f1, b.f1, t.thousand, t.tenthous from (select sum(f1) as f1 from int4_tbl i4b) b where b.f1 = t.thousand and a.f1 = b.f1 and (a.f1+b.f1+999) = t.tenthous; +-- +-- check for correct placement of non-strict qual in a multiway outer join +-- +explain (costs off) +select t1.f1 +from int4_tbl t1, int4_tbl t2 + left join int4_tbl t3 on t3.f1 > 0 + left join int4_tbl t4 on t3.f1 > 1 +where t4.f1 is null; + +select t1.f1 +from int4_tbl t1, int4_tbl t2 + left join int4_tbl t3 on t3.f1 > 0 + left join int4_tbl t4 on t3.f1 > 1 +where t4.f1 is null; + -- -- check a case where we formerly got confused by conflicting sort orders -- in redundant merge join path keys -- 2.31.1