Skip to content

Commit 991a3df

Browse files
tglsfdctglsfdc
committed
Fix filtering of "cloned" outer-join quals some more.
We've had multiple issues with the clause_is_computable_at logic that I introduced in 2489d76: it's been known to accept more than one clone of the same qual at the same plan node, and also to accept no clones at all. It's looking impractical to get it 100% right on the basis of the currently-stored information, so fix it by introducing a new RestrictInfo field "incompatible_relids" that explicitly shows which outer joins a given clone mustn't be pushed above. In principle we could populate this field in every RestrictInfo, but that would cost space and there doesn't presently seem to be a need for it in general. Also, while deconstruct_distribute_oj_quals can easily fill the field with the remaining members of the commutative join set that it's considering, computing it in the general case seems again pretty complicated. So for now, just fill it for clone quals. Along the way, fix a bug that may or may not be only latent: equivclass.c was generating replacement clauses with is_pushed_down and has_clone/is_clone markings that didn't match their required_relids. This led me to conclude that leaving the clone flags out of make_restrictinfo's purview wasn't such a great idea after all, so add them. Per report from Richard Guo. Discussion: https://postgr.es/m/CAMbWs48EYi_9-pSd0ORes1kTmTeAjT4Q3gu49hJtYCbSn2JyeA@mail.gmail.com
1 parent 913b3da commit 991a3df

File tree

12 files changed

+178
-118
lines changed

12 files changed

+178
-118
lines changed

‎contrib/postgres_fdw/postgres_fdw.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6521,8 +6521,11 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel,
65216521
expr,
65226522
true,
65236523
false,
6524+
false,
6525+
false,
65246526
root->qual_security_level,
65256527
grouped_rel->relids,
6528+
NULL,
65266529
NULL);
65276530
if (is_foreign_expr(root, grouped_rel, expr))
65286531
fpinfo->remote_conds = lappend(fpinfo->remote_conds, rinfo);

‎src/backend/optimizer/README

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -539,14 +539,13 @@ set includes the A/B join relid which is not in the input. However,
539539
if we form B/C after A/B, then both forms of the clause are applicable
540540
so far as that test can tell. We have to look more closely to notice
541541
that the "Pbc" clause form refers to relation B which is no longer
542-
directly accessible. While this check is straightforward, it's not
543-
especially cheap (see clause_is_computable_at()). To avoid doing it
544-
unnecessarily, we mark the variant versions of a redundant clause as
545-
either "has_clone" or "is_clone". When considering a clone clause,
546-
we must check clause_is_computable_at() to disentangle which version
547-
to apply at the current join level. (In debug builds, we also Assert
548-
that non-clone clauses are validly computable at the current level;
549-
but that seems too expensive for production usage.)
542+
directly accessible. While such a check could be performed using the
543+
per-relation RelOptInfo.nulling_relids data, it would be annoyingly
544+
expensive to do over and over as we consider different join paths.
545+
To make this simple and reliable, we compute an "incompatible_relids"
546+
set for each variant version (clone) of a redundant clause. A clone
547+
clause should not be applied if any of the outer-join relids listed in
548+
incompatible_relids has already been computed below the current join.
550549

551550

552551
Optimizer Functions

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

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -197,9 +197,12 @@ process_equivalence(PlannerInfo *root,
197197
make_restrictinfo(root,
198198
(Expr *) ntest,
199199
restrictinfo->is_pushed_down,
200+
restrictinfo->has_clone,
201+
restrictinfo->is_clone,
200202
restrictinfo->pseudoconstant,
201203
restrictinfo->security_level,
202204
NULL,
205+
restrictinfo->incompatible_relids,
203206
restrictinfo->outer_relids);
204207
}
205208
return false;
@@ -1972,7 +1975,8 @@ create_join_clause(PlannerInfo *root,
19721975
* clause into the regular processing, because otherwise the join will be
19731976
* seen as a clauseless join and avoided during join order searching.
19741977
* We handle this by generating a constant-TRUE clause that is marked with
1975-
* required_relids that make it a join between the correct relations.
1978+
* the same required_relids etc as the removed outer-join clause, thus
1979+
* making it a join clause between the correct relations.
19761980
*/
19771981
void
19781982
reconsider_outer_join_clauses(PlannerInfo *root)
@@ -2001,10 +2005,13 @@ reconsider_outer_join_clauses(PlannerInfo *root)
20012005
/* throw back a dummy replacement clause (see notes above) */
20022006
rinfo = make_restrictinfo(root,
20032007
(Expr *) makeBoolConst(true, false),
2004-
true, /* is_pushed_down */
2008+
rinfo->is_pushed_down,
2009+
rinfo->has_clone,
2010+
rinfo->is_clone,
20052011
false, /* pseudoconstant */
20062012
0, /* security_level */
20072013
rinfo->required_relids,
2014+
rinfo->incompatible_relids,
20082015
rinfo->outer_relids);
20092016
distribute_restrictinfo_to_rels(root, rinfo);
20102017
}
@@ -2026,10 +2033,13 @@ reconsider_outer_join_clauses(PlannerInfo *root)
20262033
/* throw back a dummy replacement clause (see notes above) */
20272034
rinfo = make_restrictinfo(root,
20282035
(Expr *) makeBoolConst(true, false),
2029-
true, /* is_pushed_down */
2036+
rinfo->is_pushed_down,
2037+
rinfo->has_clone,
2038+
rinfo->is_clone,
20302039
false, /* pseudoconstant */
20312040
0, /* security_level */
20322041
rinfo->required_relids,
2042+
rinfo->incompatible_relids,
20332043
rinfo->outer_relids);
20342044
distribute_restrictinfo_to_rels(root, rinfo);
20352045
}
@@ -2051,10 +2061,13 @@ reconsider_outer_join_clauses(PlannerInfo *root)
20512061
/* throw back a dummy replacement clause (see notes above) */
20522062
rinfo = make_restrictinfo(root,
20532063
(Expr *) makeBoolConst(true, false),
2054-
true, /* is_pushed_down */
2064+
rinfo->is_pushed_down,
2065+
rinfo->has_clone,
2066+
rinfo->is_clone,
20552067
false, /* pseudoconstant */
20562068
0, /* security_level */
20572069
rinfo->required_relids,
2070+
rinfo->incompatible_relids,
20582071
rinfo->outer_relids);
20592072
distribute_restrictinfo_to_rels(root, rinfo);
20602073
}

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

Lines changed: 47 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ static void distribute_quals_to_rels(PlannerInfo *root, List *clauses,
109109
Relids qualscope,
110110
Relids ojscope,
111111
Relids outerjoin_nonnullable,
112+
Relids incompatible_relids,
112113
bool allow_equivalence,
113114
bool has_clone,
114115
bool is_clone,
@@ -120,6 +121,7 @@ static void distribute_qual_to_rels(PlannerInfo *root, Node *clause,
120121
Relids qualscope,
121122
Relids ojscope,
122123
Relids outerjoin_nonnullable,
124+
Relids incompatible_relids,
123125
bool allow_equivalence,
124126
bool has_clone,
125127
bool is_clone,
@@ -1132,7 +1134,8 @@ deconstruct_distribute(PlannerInfo *root, JoinTreeItem *jtitem)
11321134
jtitem,
11331135
NULL,
11341136
root->qual_security_level,
1135-
jtitem->qualscope, NULL, NULL,
1137+
jtitem->qualscope,
1138+
NULL, NULL, NULL,
11361139
true, false, false,
11371140
NULL);
11381141

@@ -1143,7 +1146,8 @@ deconstruct_distribute(PlannerInfo *root, JoinTreeItem *jtitem)
11431146
jtitem,
11441147
NULL,
11451148
root->qual_security_level,
1146-
jtitem->qualscope, NULL, NULL,
1149+
jtitem->qualscope,
1150+
NULL, NULL, NULL,
11471151
true, false, false,
11481152
NULL);
11491153
}
@@ -1226,6 +1230,7 @@ deconstruct_distribute(PlannerInfo *root, JoinTreeItem *jtitem)
12261230
root->qual_security_level,
12271231
jtitem->qualscope,
12281232
ojscope, jtitem->nonnullable_rels,
1233+
NULL, /* incompatible_relids */
12291234
true, /* allow_equivalence */
12301235
false, false, /* not clones */
12311236
postponed_oj_qual_list);
@@ -1285,6 +1290,7 @@ process_security_barrier_quals(PlannerInfo *root,
12851290
jtitem->qualscope,
12861291
jtitem->qualscope,
12871292
NULL,
1293+
NULL,
12881294
true,
12891295
false, false, /* not clones */
12901296
NULL);
@@ -1887,6 +1893,7 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
18871893
{
18881894
Relids joins_above;
18891895
Relids joins_below;
1896+
Relids incompatible_joins;
18901897
Relids joins_so_far;
18911898
List *quals;
18921899
int save_last_rinfo_serial;
@@ -1920,6 +1927,15 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
19201927
joins_below,
19211928
NULL);
19221929

1930+
/*
1931+
* We'll need to mark the lower versions of the quals as not safe to
1932+
* apply above not-yet-processed joins of the stack. This prevents
1933+
* possibly applying a cloned qual at the wrong join level.
1934+
*/
1935+
incompatible_joins = bms_union(joins_below, joins_above);
1936+
incompatible_joins = bms_add_member(incompatible_joins,
1937+
sjinfo->ojrelid);
1938+
19231939
/*
19241940
* Each time we produce RestrictInfo(s) from these quals, reset the
19251941
* last_rinfo_serial counter, so that the RestrictInfos for the "same"
@@ -1979,13 +1995,19 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
19791995
* relation B will appear nulled by the syntactically-upper OJ
19801996
* within the Pbc clause, but those of relation C will not. (In
19811997
* the notation used by optimizer/README, we're converting a qual
1982-
* of the form Pbc to Pb*c.)
1998+
* of the form Pbc to Pb*c.) Of course, we must also remove that
1999+
* bit from the incompatible_joins value, else we'll make a qual
2000+
* that can't be placed anywhere.
19832001
*/
19842002
if (above_sjinfo)
2003+
{
19852004
quals = (List *)
19862005
add_nulling_relids((Node *) quals,
19872006
sjinfo->syn_lefthand,
19882007
bms_make_singleton(othersj->ojrelid));
2008+
incompatible_joins = bms_del_member(incompatible_joins,
2009+
othersj->ojrelid);
2010+
}
19892011

19902012
/* Compute qualscope and ojscope for this join level */
19912013
this_qualscope = bms_union(qualscope, joins_so_far);
@@ -2027,6 +2049,7 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
20272049
root->qual_security_level,
20282050
this_qualscope,
20292051
this_ojscope, nonnullable_rels,
2052+
bms_copy(incompatible_joins),
20302053
allow_equivalence,
20312054
has_clone,
20322055
is_clone,
@@ -2039,13 +2062,17 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
20392062
* Vars coming from the lower join's RHS. (Again, we are
20402063
* converting a qual of the form Pbc to Pb*c, but now we are
20412064
* putting back bits that were there in the parser output and were
2042-
* temporarily stripped above.)
2065+
* temporarily stripped above.) Update incompatible_joins too.
20432066
*/
20442067
if (below_sjinfo)
2068+
{
20452069
quals = (List *)
20462070
add_nulling_relids((Node *) quals,
20472071
othersj->syn_righthand,
20482072
bms_make_singleton(othersj->ojrelid));
2073+
incompatible_joins = bms_del_member(incompatible_joins,
2074+
othersj->ojrelid);
2075+
}
20492076

20502077
/* ... and track joins processed so far */
20512078
joins_so_far = bms_add_member(joins_so_far, othersj->ojrelid);
@@ -2060,6 +2087,7 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
20602087
root->qual_security_level,
20612088
qualscope,
20622089
ojscope, nonnullable_rels,
2090+
NULL, /* incompatible_relids */
20632091
true, /* allow_equivalence */
20642092
false, false, /* not clones */
20652093
NULL); /* no more postponement */
@@ -2086,6 +2114,7 @@ distribute_quals_to_rels(PlannerInfo *root, List *clauses,
20862114
Relids qualscope,
20872115
Relids ojscope,
20882116
Relids outerjoin_nonnullable,
2117+
Relids incompatible_relids,
20892118
bool allow_equivalence,
20902119
bool has_clone,
20912120
bool is_clone,
@@ -2104,6 +2133,7 @@ distribute_quals_to_rels(PlannerInfo *root, List *clauses,
21042133
qualscope,
21052134
ojscope,
21062135
outerjoin_nonnullable,
2136+
incompatible_relids,
21072137
allow_equivalence,
21082138
has_clone,
21092139
is_clone,
@@ -2135,6 +2165,9 @@ distribute_quals_to_rels(PlannerInfo *root, List *clauses,
21352165
* base+OJ rels appearing on the outer (nonnullable) side of the join
21362166
* (for FULL JOIN this includes both sides of the join, and must in fact
21372167
* equal qualscope)
2168+
* 'incompatible_relids': the set of outer-join relid(s) that must not be
2169+
* computed below this qual. We only bother to compute this for
2170+
* "clone" quals, otherwise it can be left NULL.
21382171
* 'allow_equivalence': true if it's okay to convert clause into an
21392172
* EquivalenceClass
21402173
* 'has_clone': has_clone property to assign to the qual
@@ -2159,6 +2192,7 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause,
21592192
Relids qualscope,
21602193
Relids ojscope,
21612194
Relids outerjoin_nonnullable,
2195+
Relids incompatible_relids,
21622196
bool allow_equivalence,
21632197
bool has_clone,
21642198
bool is_clone,
@@ -2377,15 +2411,14 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause,
23772411
restrictinfo = make_restrictinfo(root,
23782412
(Expr *) clause,
23792413
is_pushed_down,
2414+
has_clone,
2415+
is_clone,
23802416
pseudoconstant,
23812417
security_level,
23822418
relids,
2419+
incompatible_relids,
23832420
outerjoin_nonnullable);
23842421

2385-
/* Apply appropriate clone marking, too */
2386-
restrictinfo->has_clone = has_clone;
2387-
restrictinfo->is_clone = is_clone;
2388-
23892422
/*
23902423
* If it's a join clause, add vars used in the clause to targetlists of
23912424
* their relations, so that they will be emitted by the plan nodes that
@@ -2750,9 +2783,12 @@ process_implied_equality(PlannerInfo *root,
27502783
restrictinfo = make_restrictinfo(root,
27512784
(Expr *) clause,
27522785
true, /* is_pushed_down */
2786+
false, /* !has_clone */
2787+
false, /* !is_clone */
27532788
pseudoconstant,
27542789
security_level,
27552790
relids,
2791+
NULL, /* incompatible_relids */
27562792
NULL); /* outer_relids */
27572793

27582794
/*
@@ -2841,9 +2877,12 @@ build_implied_join_equality(PlannerInfo *root,
28412877
restrictinfo = make_restrictinfo(root,
28422878
clause,
28432879
true, /* is_pushed_down */
2880+
false, /* !has_clone */
2881+
false, /* !is_clone */
28442882
false, /* pseudoconstant */
28452883
security_level, /* security_level */
28462884
qualscope, /* required_relids */
2885+
NULL, /* incompatible_relids */
28472886
NULL); /* outer_relids */
28482887

28492888
/* Set mergejoinability/hashjoinability flags */

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -894,9 +894,11 @@ apply_child_basequals(PlannerInfo *root, RelOptInfo *parentrel,
894894
make_restrictinfo(root,
895895
(Expr *) onecq,
896896
rinfo->is_pushed_down,
897+
rinfo->has_clone,
898+
rinfo->is_clone,
897899
pseudoconstant,
898900
rinfo->security_level,
899-
NULL, NULL));
901+
NULL, NULL, NULL));
900902
/* track minimum security level among child quals */
901903
cq_min_security = Min(cq_min_security, rinfo->security_level);
902904
}
@@ -929,9 +931,11 @@ apply_child_basequals(PlannerInfo *root, RelOptInfo *parentrel,
929931
/* not likely that we'd see constants here, so no check */
930932
childquals = lappend(childquals,
931933
make_restrictinfo(root, qual,
932-
true, false,
934+
true,
935+
false, false,
936+
false,
933937
security_level,
934-
NULL, NULL));
938+
NULL, NULL, NULL));
935939
cq_min_security = Min(cq_min_security, security_level);
936940
}
937941
security_level++;

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,8 +267,11 @@ consider_new_or_clause(PlannerInfo *root, RelOptInfo *rel,
267267
orclause,
268268
true,
269269
false,
270+
false,
271+
false,
270272
join_or_rinfo->security_level,
271273
NULL,
274+
NULL,
272275
NULL);
273276

274277
/*

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

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1346,27 +1346,21 @@ subbuild_joinrel_restrictlist(PlannerInfo *root,
13461346
Assert(!RINFO_IS_PUSHED_DOWN(rinfo, joinrel->relids));
13471347
if (!bms_is_subset(rinfo->required_relids, both_input_relids))
13481348
continue;
1349-
if (!clause_is_computable_at(root, rinfo, both_input_relids))
1349+
if (bms_overlap(rinfo->incompatible_relids, both_input_relids))
13501350
continue;
13511351
}
13521352
else
13531353
{
13541354
/*
13551355
* For non-clone clauses, we just Assert it's OK. These might
1356-
* be either join or filter clauses.
1356+
* be either join or filter clauses; if it's a join clause
1357+
* then it should not refer to the current join's output.
1358+
* (There is little point in checking incompatible_relids,
1359+
* because it'll be NULL.)
13571360
*/
1358-
#ifdef USE_ASSERT_CHECKING
1359-
if (RINFO_IS_PUSHED_DOWN(rinfo, joinrel->relids))
1360-
Assert(clause_is_computable_at(root, rinfo,
1361-
joinrel->relids));
1362-
else
1363-
{
1364-
Assert(bms_is_subset(rinfo->required_relids,
1365-
both_input_relids));
1366-
Assert(clause_is_computable_at(root, rinfo,
1367-
both_input_relids));
1368-
}
1369-
#endif
1361+
Assert(RINFO_IS_PUSHED_DOWN(rinfo, joinrel->relids) ||
1362+
bms_is_subset(rinfo->required_relids,
1363+
both_input_relids));
13701364
}
13711365

13721366
/*

0 commit comments

Comments
 (0)