Skip to content

Commit 447dad7

Browse files
tglsfdctglsfdc
committed
Fix planning of non-strict equivalence clauses above outer joins.
If a potential equivalence clause references a variable from the nullable side of an outer join, the planner needs to take care that derived clauses are not pushed to below the outer join; else they may use the wrong value for the variable. (The problem arises only with non-strict clauses, since if an upper clause can be proven strict then the outer join will get simplified to a plain join.) The planner attempted to prevent this type of error by checking that potential equivalence clauses aren't outerjoin-delayed as a whole, but actually we have to check each side separately, since the two sides of the clause will get moved around separately if it's treated as an equivalence. Bugs of this type can be demonstrated as far back as 7.4, even though releases before 8.3 had only a very ad-hoc notion of equivalence clauses. In addition, we neglected to account for the possibility that such clauses might have nonempty nullable_relids even when not outerjoin-delayed; so the equivalence-class machinery lacked logic to compute correct nullable_relids values for clauses it constructs. This oversight was harmless before 9.2 because we were only using RestrictInfo.nullable_relids for OR clauses; but as of 9.2 it could result in pushing constructed equivalence clauses to incorrect places. (This accounts for bug #7604 from Bill MacArthur.) Fix the first problem by adding a new test check_equivalence_delay() in distribute_qual_to_rels, and fix the second one by adding code in equivclass.c and called functions to set correct nullable_relids for generated clauses. Although I believe the second part of this is not currently necessary before 9.2, I chose to back-patch it anyway, partly to keep the logic similar across branches and partly because it seems possible we might find other reasons why we need valid values of nullable_relids in the older branches. Add regression tests illustrating these problems. In 9.0 and up, also add test cases checking that we can push constants through outer joins, since we've broken that optimization before and I nearly broke it again with an overly simplistic patch for this problem.
1 parent 473320e commit 447dad7

File tree

7 files changed

+249
-36
lines changed

7 files changed

+249
-36
lines changed

‎src/backend/nodes/outfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1801,6 +1801,7 @@ _outEquivalenceMember(StringInfo str, EquivalenceMember *node)
18011801

18021802
WRITE_NODE_FIELD(em_expr);
18031803
WRITE_BITMAPSET_FIELD(em_relids);
1804+
WRITE_BITMAPSET_FIELD(em_nullable_relids);
18041805
WRITE_BOOL_FIELD(em_is_const);
18051806
WRITE_BOOL_FIELD(em_is_child);
18061807
WRITE_OID_FIELD(em_datatype);

‎src/backend/optimizer/path/equivclass.c

Lines changed: 64 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030

3131

3232
static EquivalenceMember *add_eq_member(EquivalenceClass *ec,
33-
Expr *expr, Relids relids,
33+
Expr *expr, Relids relids, Relids nullable_relids,
3434
bool is_child, Oid datatype);
3535
static void generate_base_implied_equalities_const(PlannerInfo *root,
3636
EquivalenceClass *ec);
@@ -105,7 +105,9 @@ process_equivalence(PlannerInfo *root, RestrictInfo *restrictinfo,
105105
Expr *item1;
106106
Expr *item2;
107107
Relids item1_relids,
108-
item2_relids;
108+
item2_relids,
109+
item1_nullable_relids,
110+
item2_nullable_relids;
109111
List *opfamilies;
110112
EquivalenceClass *ec1,
111113
*ec2;
@@ -162,6 +164,12 @@ process_equivalence(PlannerInfo *root, RestrictInfo *restrictinfo,
162164
return false; /* RHS is non-strict but not constant */
163165
}
164166

167+
/* Calculate nullable-relid sets for each side of the clause */
168+
item1_nullable_relids = bms_intersect(item1_relids,
169+
restrictinfo->nullable_relids);
170+
item2_nullable_relids = bms_intersect(item2_relids,
171+
restrictinfo->nullable_relids);
172+
165173
/*
166174
* We use the declared input types of the operator, not exprType() of the
167175
* inputs, as the nominal datatypes for opfamily lookup. This presumes
@@ -308,7 +316,8 @@ process_equivalence(PlannerInfo *root, RestrictInfo *restrictinfo,
308316
else if (ec1)
309317
{
310318
/* Case 3: add item2 to ec1 */
311-
em2 = add_eq_member(ec1, item2, item2_relids, false, item2_type);
319+
em2 = add_eq_member(ec1, item2, item2_relids, item2_nullable_relids,
320+
false, item2_type);
312321
ec1->ec_sources = lappend(ec1->ec_sources, restrictinfo);
313322
ec1->ec_below_outer_join |= below_outer_join;
314323
/* mark the RI as associated with this eclass */
@@ -321,7 +330,8 @@ process_equivalence(PlannerInfo *root, RestrictInfo *restrictinfo,
321330
else if (ec2)
322331
{
323332
/* Case 3: add item1 to ec2 */
324-
em1 = add_eq_member(ec2, item1, item1_relids, false, item1_type);
333+
em1 = add_eq_member(ec2, item1, item1_relids, item1_nullable_relids,
334+
false, item1_type);
325335
ec2->ec_sources = lappend(ec2->ec_sources, restrictinfo);
326336
ec2->ec_below_outer_join |= below_outer_join;
327337
/* mark the RI as associated with this eclass */
@@ -348,8 +358,10 @@ process_equivalence(PlannerInfo *root, RestrictInfo *restrictinfo,
348358
ec->ec_broken = false;
349359
ec->ec_sortref = 0;
350360
ec->ec_merged = NULL;
351-
em1 = add_eq_member(ec, item1, item1_relids, false, item1_type);
352-
em2 = add_eq_member(ec, item2, item2_relids, false, item2_type);
361+
em1 = add_eq_member(ec, item1, item1_relids, item1_nullable_relids,
362+
false, item1_type);
363+
em2 = add_eq_member(ec, item2, item2_relids, item2_nullable_relids,
364+
false, item2_type);
353365

354366
root->eq_classes = lappend(root->eq_classes, ec);
355367

@@ -447,12 +459,13 @@ canonicalize_ec_expression(Expr *expr, Oid req_type, Oid req_collation)
447459
*/
448460
static EquivalenceMember *
449461
add_eq_member(EquivalenceClass *ec, Expr *expr, Relids relids,
450-
bool is_child, Oid datatype)
462+
Relids nullable_relids, bool is_child, Oid datatype)
451463
{
452464
EquivalenceMember *em = makeNode(EquivalenceMember);
453465

454466
em->em_expr = expr;
455467
em->em_relids = relids;
468+
em->em_nullable_relids = nullable_relids;
456469
em->em_is_const = false;
457470
em->em_is_child = is_child;
458471
em->em_datatype = datatype;
@@ -608,7 +621,7 @@ get_eclass_for_sort_expr(PlannerInfo *root,
608621
elog(ERROR, "volatile EquivalenceClass has no sortref");
609622

610623
newem = add_eq_member(newec, copyObject(expr), pull_varnos((Node *) expr),
611-
false, opcintype);
624+
NULL, false, opcintype);
612625

613626
/*
614627
* add_eq_member doesn't check for volatile functions, set-returning
@@ -788,7 +801,9 @@ generate_base_implied_equalities_const(PlannerInfo *root,
788801
}
789802
process_implied_equality(root, eq_op, ec->ec_collation,
790803
cur_em->em_expr, const_em->em_expr,
791-
ec->ec_relids,
804+
bms_copy(ec->ec_relids),
805+
bms_union(cur_em->em_nullable_relids,
806+
const_em->em_nullable_relids),
792807
ec->ec_below_outer_join,
793808
cur_em->em_is_const);
794809
}
@@ -843,7 +858,9 @@ generate_base_implied_equalities_no_const(PlannerInfo *root,
843858
}
844859
process_implied_equality(root, eq_op, ec->ec_collation,
845860
prev_em->em_expr, cur_em->em_expr,
846-
ec->ec_relids,
861+
bms_copy(ec->ec_relids),
862+
bms_union(prev_em->em_nullable_relids,
863+
cur_em->em_nullable_relids),
847864
ec->ec_below_outer_join,
848865
false);
849866
}
@@ -1248,7 +1265,9 @@ create_join_clause(PlannerInfo *root,
12481265
leftem->em_expr,
12491266
rightem->em_expr,
12501267
bms_union(leftem->em_relids,
1251-
rightem->em_relids));
1268+
rightem->em_relids),
1269+
bms_union(leftem->em_nullable_relids,
1270+
rightem->em_nullable_relids));
12521271

12531272
/* Mark the clause as redundant, or not */
12541273
rinfo->parent_ec = parent_ec;
@@ -1470,7 +1489,8 @@ reconsider_outer_join_clause(PlannerInfo *root, RestrictInfo *rinfo,
14701489
left_type,
14711490
right_type,
14721491
inner_datatype;
1473-
Relids inner_relids;
1492+
Relids inner_relids,
1493+
inner_nullable_relids;
14741494
ListCell *lc1;
14751495

14761496
Assert(is_opclause(rinfo->clause));
@@ -1497,6 +1517,8 @@ reconsider_outer_join_clause(PlannerInfo *root, RestrictInfo *rinfo,
14971517
inner_datatype = left_type;
14981518
inner_relids = rinfo->left_relids;
14991519
}
1520+
inner_nullable_relids = bms_intersect(inner_relids,
1521+
rinfo->nullable_relids);
15001522

15011523
/* Scan EquivalenceClasses for a match to outervar */
15021524
foreach(lc1, root->eq_classes)
@@ -1555,7 +1577,8 @@ reconsider_outer_join_clause(PlannerInfo *root, RestrictInfo *rinfo,
15551577
cur_ec->ec_collation,
15561578
innervar,
15571579
cur_em->em_expr,
1558-
inner_relids);
1580+
bms_copy(inner_relids),
1581+
bms_copy(inner_nullable_relids));
15591582
if (process_equivalence(root, newrinfo, true))
15601583
match = true;
15611584
}
@@ -1589,7 +1612,9 @@ reconsider_full_join_clause(PlannerInfo *root, RestrictInfo *rinfo)
15891612
left_type,
15901613
right_type;
15911614
Relids left_relids,
1592-
right_relids;
1615+
right_relids,
1616+
left_nullable_relids,
1617+
right_nullable_relids;
15931618
ListCell *lc1;
15941619

15951620
/* Can't use an outerjoin_delayed clause here */
@@ -1605,6 +1630,10 @@ reconsider_full_join_clause(PlannerInfo *root, RestrictInfo *rinfo)
16051630
rightvar = (Expr *) get_rightop(rinfo->clause);
16061631
left_relids = rinfo->left_relids;
16071632
right_relids = rinfo->right_relids;
1633+
left_nullable_relids = bms_intersect(left_relids,
1634+
rinfo->nullable_relids);
1635+
right_nullable_relids = bms_intersect(right_relids,
1636+
rinfo->nullable_relids);
16081637

16091638
foreach(lc1, root->eq_classes)
16101639
{
@@ -1690,7 +1719,8 @@ reconsider_full_join_clause(PlannerInfo *root, RestrictInfo *rinfo)
16901719
cur_ec->ec_collation,
16911720
leftvar,
16921721
cur_em->em_expr,
1693-
left_relids);
1722+
bms_copy(left_relids),
1723+
bms_copy(left_nullable_relids));
16941724
if (process_equivalence(root, newrinfo, true))
16951725
matchleft = true;
16961726
}
@@ -1703,7 +1733,8 @@ reconsider_full_join_clause(PlannerInfo *root, RestrictInfo *rinfo)
17031733
cur_ec->ec_collation,
17041734
rightvar,
17051735
cur_em->em_expr,
1706-
right_relids);
1736+
bms_copy(right_relids),
1737+
bms_copy(right_nullable_relids));
17071738
if (process_equivalence(root, newrinfo, true))
17081739
matchright = true;
17091740
}
@@ -1829,11 +1860,27 @@ add_child_rel_equivalences(PlannerInfo *root,
18291860
{
18301861
/* Yes, generate transformed child version */
18311862
Expr *child_expr;
1863+
Relids new_nullable_relids;
18321864

18331865
child_expr = (Expr *)
18341866
adjust_appendrel_attrs((Node *) cur_em->em_expr,
18351867
appinfo);
1836-
(void) add_eq_member(cur_ec, child_expr, child_rel->relids,
1868+
1869+
/*
1870+
* Must translate nullable_relids. Note this code assumes
1871+
* parent and child relids are singletons.
1872+
*/
1873+
new_nullable_relids = cur_em->em_nullable_relids;
1874+
if (bms_overlap(new_nullable_relids, parent_rel->relids))
1875+
{
1876+
new_nullable_relids = bms_difference(new_nullable_relids,
1877+
parent_rel->relids);
1878+
new_nullable_relids = bms_add_members(new_nullable_relids,
1879+
child_rel->relids);
1880+
}
1881+
1882+
(void) add_eq_member(cur_ec, child_expr,
1883+
child_rel->relids, new_nullable_relids,
18371884
true, cur_em->em_datatype);
18381885
}
18391886
}

0 commit comments

Comments
 (0)