8000 Change more places to be less trusting of RestrictInfo.is_pushed_down. · tomdcc/postgres@9680c12 · GitHub
[go: up one dir, main page]
Skip to content

Commit 9680c12

Browse files
tglsfdctglsfdc
committed
Change more places to be less trusting of RestrictInfo.is_pushed_down.
On further reflection, commit e5d8399 didn't go far enough: pretty much everywhere in the planner that examines a clause's is_pushed_down flag ought to be changed to use the more complicated behavior where we also check the clause's required_relids. Otherwise we could make incorrect decisions about whether, say, a clause is safe to use as a hash clause. Some (many?) of these places are safe as-is, either because they are never reached while considering a parameterized path, or because there are additional checks that would reject a pushed-down clause anyway. However, it seems smarter to just code them all the same way rather than rely on easily-broken reasoning of that sort. In support of that, invent a new macro RINFO_IS_PUSHED_DOWN that should be used in place of direct tests on the is_pushed_down flag. Like the previous patch, back-patch to all supported branches. Discussion: https://postgr.es/m/f8128b11-c5bf-3539-48cd-234178b2314d@proxel.se
1 parent e1d4398 commit 9680c12

File tree

7 files changed

+52
-32
lines changed

7 files changed

+52
-32
lines changed

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,7 @@ static bool has_indexed_join_quals(NestPath *joinpath);
137137
static double approx_tuple_count(PlannerInfo *root, JoinPath *path,
138138
List *quals);
139139
static double calc_joinrel_size_estimate(PlannerInfo *root,
140+
RelOptInfo *joinrel,
140141
double outer_rows,
141142
double inner_rows,
142143
SpecialJoinInfo *sjinfo,
@@ -3244,13 +3245,15 @@ compute_semi_anti_join_factors(PlannerInfo *root,
32443245
*/
32453246
if (jointype == JOIN_ANTI)
32463247
{
3248+
Relids joinrelids = bms_union(outerrel->relids, innerrel->relids);
3249+
32473250
joinquals = NIL;
32483251
foreach(l, restrictlist)
32493252
{
32503253
RestrictInfo *rinfo = (RestrictInfo *) lfirst(l);
32513254

32523255
Assert(IsA(rinfo, RestrictInfo));
3253-
if (!rinfo->is_pushed_down)
3256+
if (!RINFO_IS_PUSHED_DOWN(rinfo, joinrelids))
32543257
joinquals = lappend(joinquals, rinfo);
32553258
}
32563259
}
@@ -3559,6 +3562,7 @@ set_joinrel_size_estimates(PlannerInfo *root, RelOptInfo *rel,
35593562
List *restrictlist)
35603563
{
35613564
rel->rows = calc_joinrel_size_estimate(root,
3565+
rel,
35623566
outer_rel->rows,
35633567
inner_rel->rows,
35643568
sjinfo,
@@ -3599,6 +3603,7 @@ get_parameterized_joinrel_size(PlannerInfo *root, RelOptInfo *rel,
35993603
* estimate for any pair with the same parameterization.
36003604
*/
36013605
nrows = calc_joinrel_size_estimate(root,
3606+
rel,
36023607
outer_rows,
36033608
inner_rows,
36043609
sjinfo,
@@ -3616,6 +3621,7 @@ get_parameterized_joinrel_size(PlannerInfo *root, RelOptInfo *rel,
36163621
*/
36173622
static double
36183623
calc_joinrel_size_estimate(PlannerInfo *root,
3624+
RelOptInfo *joinrel,
36193625
double outer_rows,
36203626
double inner_rows,
36213627
SpecialJoinInfo *sjinfo,
@@ -3648,7 +3654,7 @@ calc_joinrel_size_estimate(PlannerInfo *root,
36483654
RestrictInfo *rinfo = (RestrictInfo *) lfirst(l);
36493655

36503656
Assert(IsA(rinfo, RestrictInfo));
3651-
if (rinfo->is_pushed_down)
3657+
if (RINFO_IS_PUSHED_DOWN(rinfo, joinrel->relids))
36523658
pushedquals = lappend(pushedquals, rinfo);
36533659
else
36543660
joinquals = lappend(joinquals, rinfo);

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1145,7 +1145,7 @@ hash_inner_and_outer(PlannerInfo *root,
11451145
* If processing an outer join, only use its own join clauses for
11461146
* hashing. For inner joins we need not be so picky.
11471147
*/
1148-
if (isouterjoin && restrictinfo->is_pushed_down)
1148+
if (isouterjoin && RINFO_IS_PUSHED_DOWN(restrictinfo, joinrel->relids))
11491149
continue;
11501150

11511151
if (!restrictinfo->can_join ||
@@ -1345,7 +1345,7 @@ select_mergejoin_clauses(PlannerInfo *root,
13451345
* we don't set have_nonmergeable_joinclause here because pushed-down
13461346
* clauses will become otherquals not joinquals.)
13471347
*/
1348-
if (isouterjoin && restrictinfo->is_pushed_down)
1348+
if (isouterjoin && RINFO_IS_PUSHED_DOWN(restrictinfo, joinrel->relids))
13491349
continue;
13501350

13511351
/* Check that clause is a mergeable operator clause */

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

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ static bool has_legal_joinclause(PlannerInfo *root, RelOptInfo *rel);
3131
static bool is_dummy_rel(RelOptInfo *rel);
3232
static void mark_dummy_rel(RelOptInfo *rel);
3333
static bool restriction_is_constant_false(List *restrictlist,
34+
RelOptInfo *joinrel,
3435
bool only_pushed_down);
3536

3637

@@ -759,7 +760,7 @@ make_join_rel(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2)
759760
{
760761
case JOIN_INNER:
761762
if (is_dummy_rel(rel1) || is_dummy_rel(rel2) ||
762-
restriction_is_constant_false(restrictlist, false))
763+
restriction_is_constant_false(restrictlist, joinrel, false))
763764
{
764765
mark_dummy_rel(joinrel);
765766
break;
@@ -773,12 +774,12 @@ make_join_rel(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2)
773774
break;
774775
case JOIN_LEFT:
775776
if (is_dummy_rel(rel1) ||
776-
restriction_is_constant_false(restrictlist, true))
777+
restriction_is_constant_false(restrictlist, joinrel, true))
777778
{
778779
mark_dummy_rel(joinrel);
779780
break;
780781
}
781-
if (restriction_is_constant_false(restrictlist, false) &&
782+
if (restriction_is_constant_false(restrictlist, joinrel, false) &&
782783
bms_is_subset(rel2->relids, sjinfo->syn_righthand))
783784
mark_dummy_rel(rel2);
784785
add_paths_to_joinrel(root, joinrel, rel1, rel2,
@@ -790,7 +791,7 @@ make_join_rel(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2)
790791
break;
791792
case JOIN_FULL:
792793
if ((is_dummy_rel(rel1) && is_dummy_rel(rel2)) ||
793-
restriction_is_constant_false(restrictlist, true))
794+
restriction_is_constant_false(restrictlist, joinrel, true))
794795
{
795796
mark_dummy_rel(joinrel);
796797
break;
@@ -826,7 +827,7 @@ make_join_rel(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2)
826827
bms_is_subset(sjinfo->min_righthand, rel2->relids))
827828
{
828829
if (is_dummy_rel(rel1) || is_dummy_rel(rel2) ||
829-
restriction_is_constant_false(restrictlist, false))
830+
restriction_is_constant_false(restrictlist, joinrel, false))
830831
{
831832
mark_dummy_rel(joinrel);
832833
break;
@@ -849,7 +850,7 @@ make_join_rel(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2)
849850
sjinfo) != NULL)
850851
{
851852
if (is_dummy_rel(rel1) || is_dummy_rel(rel2) ||
852-
restriction_is_constant_false(restrictlist, false))
853+
restriction_is_constant_false(restrictlist, joinrel, false))
853854
{
854855
mark_dummy_rel(joinrel);
855856
break;
@@ -864,12 +865,12 @@ make_join_rel(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2)
864865
break;
865866
case JOIN_ANTI:
866867
if (is_dummy_rel(rel1) ||
867-
restriction_is_constant_false(restrictlist, true))
868+
restriction_is_constant_false(restrictlist, joinrel, true))
868869
{
869870
mark_dummy_rel(joinrel);
870871
break;
871872
}
872-
if (restriction_is_constant_false(restrictlist, false) &&
873+
if (restriction_is_constant_false(restrictlist, joinrel, false) &&
873874
bms_is_subset(rel2->relids, sjinfo->syn_righthand))
874875
mark_dummy_rel(rel2);
875876
add_paths_to_joinrel(root, joinrel, rel1, rel2,
@@ -1227,18 +1228,21 @@ mark_dummy_rel(RelOptInfo *rel)
12271228

12281229

12291230
/*
1230-
* restriction_is_constant_false --- is a restrictlist just FALSE?
1231+
* restriction_is_constant_false --- is a restrictlist just false?
12311232
*
1232-
* In cases where a qual is provably constant FALSE, eval_const_expressions
1233+
* In cases where a qual is provably constant false, eval_const_expressions
12331234
* will generally have thrown away anything that's ANDed with it. In outer
12341235
* join situations this will leave us computing cartesian products only to
12351236
* decide there's no match for an outer row, which is pretty stupid. So,
12361237
* we need to detect the case.
12371238
*
1238-
* If only_pushed_down is TRUE, then consider only pushed-down quals.
1239+
* If only_pushed_down is true, then consider only quals that are pushed-down
1240+
* from the point of view of the joinrel.
12391241
*/
12401242
static bool
1241-
restriction_is_constant_false(List *restrictlist, bool only_pushed_down)
1243+
restriction_is_constant_false(List *restrictlist,
1244+
RelOptInfo *joinrel,
1245+
bool only_pushed_down)
12421246
{
12431247
ListCell *lc;
12441248

@@ -1253,7 +1257,7 @@ restriction_is_constant_false(List *restrictlist, bool only_pushed_down)
12531257
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
12541258

12551259
Assert(IsA(rinfo, RestrictInfo));
1256-
if (only_pushed_down && !rinfo->is_pushed_down)
1260+
if (only_pushed_down && !RINFO_IS_PUSHED_DOWN(rinfo, joinrel->relids))
12571261
continue;
12581262

12591263
if (rinfo->clause && IsA(rinfo->clause, Const))

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -240,8 +240,7 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
240240
* above the outer join, even if it references no other rels (it might
241241
* be from WHERE, for example).
242242
*/
243-
if (restrictinfo->is_pushed_down ||
244-
!bms_equal(restrictinfo->required_relids, joinrelids))
243+
if (RINFO_IS_PUSHED_DOWN(restrictinfo, joinrelids))
245244
{
246245
/*
247246
* If such a clause actually references the inner rel then join
@@ -413,8 +412,7 @@ remove_rel_from_query(PlannerInfo *root, int relid, Relids joinrelids)
413412

414413
remove_join_clause_from_rels(root, rinfo, rinfo->required_relids);
415414

416-
if (rinfo->is_pushed_down ||
417-
!bms_equal(rinfo->required_relids, joinrelids))
415+
if (RINFO_IS_PUSHED_DOWN(rinfo, joinrelids))
418416
{
419417
/* Recheck that qual doesn't actually reference the target rel */
420418
Assert(!bms_is_member(relid, rinfo->clause_relids));

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1527,6 +1527,11 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause,
15271527
* attach quals to the lowest level where they can be evaluated. But
15281528
* if we were ever to re-introduce a mechanism for delaying evaluation
15291529
* of "expensive" quals, this area would need work.
1530+
*
1531+
* Note: generally, use of is_pushed_down has to go through the macro
1532+
* RINFO_IS_PUSHED_DOWN, because that flag alone is not always sufficient
1533+
* to tell whether a clause must be treated as pushed-down in context.
1534+
* This seems like another reason why it should perhaps be rethought.
15301535
*----------
15311536
*/
15321537
if (is_deduced)

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

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -597,7 +597,7 @@ extract_actual_clauses(List *restrictinfo_list,
597597
* extract_actual_join_clauses
598598
*
599599
* Extract bare clauses from 'restrictinfo_list', separating those that
600-
* syntactically match the join level from those that were pushed down.
600+
* semantically match the join level from those that were pushed down.
601601
* Pseudoconstant clauses are excluded from the results.
602602
*
603603
* This is only used at outer joins, since for plain joins we don't care
@@ -620,15 +620,7 @@ extract_actual_join_clauses(List *restrictinfo_list,
620620

621621
Assert(IsA(rinfo, RestrictInfo));
622622

623-
/*
624-
* We must check both is_pushed_down and required_relids, since an
625-
* outer-join clause that's been pushed down to some lower join level
626-
* via path parameterization will not be marked is_pushed_down;
627-
* nonetheless, it must be treated as a filter clause not a join
628-
* clause so far as the lower join level is concerned.
629-
*/
630-
if (rinfo->is_pushed_down ||
631-
!bms_is_subset(rinfo->required_relids, joinrelids))
623+
if (RINFO_IS_PUSHED_DOWN(rinfo, joinrelids))
632624
{
633625
if (!rinfo->pseudoconstant)
634626
*otherquals = lappend(*otherquals, rinfo->clause);

‎src/include/nodes/relation.h

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1111,7 +1111,8 @@ typedef struct HashPath
11111111
* if we decide that it can be pushed down into the nullable side of the join.
11121112
* In that case it acts as a plain filter qual for wherever it gets evaluated.
11131113
* (In short, is_pushed_down is only false for non-degenerate outer join
1114-
* conditions. Possibly we should rename it to reflect that meaning?)
1114+
* conditions. Possibly we should rename it to reflect that meaning? But
1115+
* see also the comments for RINFO_IS_PUSHED_DOWN, below.)
11151116
*
11161117
* RestrictInfo nodes also contain an outerjoin_delayed flag, which is true
11171118
* if the clause's applicability must be delayed due to any outer joins
@@ -1238,6 +1239,20 @@ typedef struct RestrictInfo
12381239
Selectivity right_bucketsize; /* avg bucketsize of right side */
12391240
} RestrictInfo;
12401241

1242+
/*
1243+
* This macro embodies the correct way to test whether a RestrictInfo is
1244+
* "pushed down" to a given outer join, that is, should be treated as a filter
1245+
* clause rather than a join clause at that outer join. This is certainly so
1246+
* if is_pushed_down is true; but examining that is not sufficient anymore,
1247+
* because outer-join clauses will get pushed down to lower outer joins when
1248+
* we generate a path for the lower outer join that is parameterized by the
1249+
* LHS of the upper one. We can detect such a clause by noting that its
1250+
* required_relids exceed the scope of the join.
1251+
*/
1252+
#define RINFO_IS_PUSHED_DOWN(rinfo, joinrelids) \
1253+
((rinfo)->is_pushed_down || \
1254+
!bms_is_subset((rinfo)->required_relids, joinrelids))
1255+
12411256
/*
12421257
* Since mergejoinscansel() is a relatively expensive function, and would
12431258
* otherwise be invoked many times while planning a large join tree,

0 commit comments

Comments
 (0)
0