Skip to content

Commit c792c7d

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 68c23cb commit c792c7d

File tree

10 files changed

+64
-37
lines changed

10 files changed

+64
-37
lines changed

‎contrib/postgres_fdw/postgres_fdw.c

+2-1
Original file line numberDiff line numberDiff line change
@@ -4705,7 +4705,8 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
47054705
bool is_remote_clause = is_foreign_expr(root, joinrel,
47064706
rinfo->clause);
47074707

4708-
if (IS_OUTER_JOIN(jointype) && !rinfo->is_pushed_down)
4708+
if (IS_OUTER_JOIN(jointype) &&
4709+
!RINFO_IS_PUSHED_DOWN(rinfo, joinrel->relids))
47094710
{
47104711
if (!is_remote_clause)
47114712
return false;

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

+8-2
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,7 @@ static bool has_indexed_join_quals(NestPath *joinpath);
159159
static double approx_tuple_count(PlannerInfo *root, JoinPath *path,
160160
List *quals);
161161
static double calc_joinrel_size_estimate(PlannerInfo *root,
162+
RelOptInfo *joinrel,
162163
RelOptInfo *outer_rel,
163164
RelOptInfo *inner_rel,
164165
double outer_rows,
@@ -4055,12 +4056,14 @@ compute_semi_anti_join_factors(PlannerInfo *root,
40554056
*/
40564057
if (IS_OUTER_JOIN(jointype))
40574058
{
4059+
Relids joinrelids = bms_union(outerrel->relids, innerrel->relids);
4060+
40584061
joinquals = NIL;
40594062
foreach(l, restrictlist)
40604063
{
40614064
RestrictInfo *rinfo = lfirst_node(RestrictInfo, l);
40624065

4063-
if (!rinfo->is_pushed_down)
4066+
if (!RINFO_IS_PUSHED_DOWN(rinfo, joinrelids))
40644067
joinquals = lappend(joinquals, rinfo);
40654068
}
40664069
}
@@ -4375,6 +4378,7 @@ set_joinrel_size_estimates(PlannerInfo *root, RelOptInfo *rel,
43754378
List *restrictlist)
43764379
{
43774380
rel->rows = calc_joinrel_size_estimate(root,
4381+
rel,
43784382
outer_rel,
43794383
inner_rel,
43804384
outer_rel->rows,
@@ -4417,6 +4421,7 @@ get_parameterized_joinrel_size(PlannerInfo *root, RelOptInfo *rel,
44174421
* estimate for any pair with the same parameterization.
44184422
*/
44194423
nrows = calc_joinrel_size_estimate(root,
4424+
rel,
44204425
outer_path->parent,
44214426
inner_path->parent,
44224427
outer_path->rows,
@@ -4440,6 +4445,7 @@ get_parameterized_joinrel_size(PlannerInfo *root, RelOptInfo *rel,
44404445
*/
44414446
static double
44424447
calc_joinrel_size_estimate(PlannerInfo *root,
4448+
RelOptInfo *joinrel,
44434449
RelOptInfo *outer_rel,
44444450
RelOptInfo *inner_rel,
44454451
double outer_rows,
@@ -4492,7 +4498,7 @@ calc_joinrel_size_estimate(PlannerInfo *root,
44924498
{
44934499
RestrictInfo *rinfo = lfirst_node(RestrictInfo, l);
44944500

4495-
if (rinfo->is_pushed_down)
4501+
if (RINFO_IS_PUSHED_DOWN(rinfo, joinrel->relids))
44964502
pushedquals = lappend(pushedquals, rinfo);
44974503
else
44984504
joinquals = lappend(joinquals, rinfo);

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

+2-2
Original file line numberDiff line numberDiff line change
@@ -1700,7 +1700,7 @@ hash_inner_and_outer(PlannerInfo *root,
17001700
* If processing an outer join, only use its own join clauses for
17011701
* hashing. For inner joins we need not be so picky.
17021702
*/
1703-
if (isouterjoin && restrictinfo->is_pushed_down)
1703+
if (isouterjoin && RINFO_IS_PUSHED_DOWN(restrictinfo, joinrel->relids))
17041704
continue;
17051705

17061706
if (!restrictinfo->can_join ||
@@ -1947,7 +1947,7 @@ select_mergejoin_clauses(PlannerInfo *root,
19471947
* we don't set have_nonmergeable_joinclause here because pushed-down
19481948
* clauses will become otherquals not joinquals.)
19491949
*/
1950-
if (isouterjoin && restrictinfo->is_pushed_down)
1950+
if (isouterjoin && RINFO_IS_PUSHED_DOWN(restrictinfo, joinrel->relids))
19511951
continue;
19521952

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

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

+20-14
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ static bool has_join_restriction(PlannerInfo *root, RelOptInfo *rel);
3535
static bool has_legal_joinclause(PlannerInfo *root, RelOptInfo *rel);
3636
static bool is_dummy_rel(RelOptInfo *rel);
3737
static bool restriction_is_constant_false(List *restrictlist,
38+
RelOptInfo *joinrel,
3839
bool only_pushed_down);
3940
static void populate_joinrel_with_paths(PlannerInfo *root, RelOptInfo *rel1,
4041
RelOptInfo *rel2, RelOptInfo *joinrel,
@@ -780,7 +781,7 @@ populate_joinrel_with_paths(PlannerInfo *root, RelOptInfo *rel1,
780781
{
781782
case JOIN_INNER:
782783
if (is_dummy_rel(rel1) || is_dummy_rel(rel2) ||
783-
restriction_is_constant_false(restrictlist, false))
784+
restriction_is_constant_false(restrictlist, joinrel, false))
784785
{
785786
mark_dummy_rel(joinrel);
786787
break;
@@ -794,12 +795,12 @@ populate_joinrel_with_paths(PlannerInfo *root, RelOptInfo *rel1,
794795
break;
795796
case JOIN_LEFT:
796797
if (is_dummy_rel(rel1) ||
797-
restriction_is_constant_false(restrictlist, true))
798+
restriction_is_constant_false(restrictlist, joinrel, true))
798799
{
799800
mark_dummy_rel(joinrel);
800801
break;
801802
}
802-
if (restriction_is_constant_false(restrictlist, false) &&
803+
if (restriction_is_constant_false(restrictlist, joinrel, false) &&
803804
bms_is_subset(rel2->relids, sjinfo->syn_righthand))
804805
mark_dummy_rel(rel2);
805806
add_paths_to_joinrel(root, joinrel, rel1, rel2,
@@ -811,7 +812,7 @@ populate_joinrel_with_paths(PlannerInfo *root, RelOptInfo *rel1,
811812
break;
812813
case JOIN_FULL:
813814
if ((is_dummy_rel(rel1) && is_dummy_rel(rel2)) ||
814-
restriction_is_constant_false(restrictlist, true))
815+
restriction_is_constant_false(restrictlist, joinrel, true))
815816
{
816817
mark_dummy_rel(joinrel);
817818
break;
@@ -847,7 +848,7 @@ populate_joinrel_with_paths(PlannerInfo *root, RelOptInfo *rel1,
847848
bms_is_subset(sjinfo->min_righthand, rel2->relids))
848849
{
849850
if (is_dummy_rel(rel1) || is_dummy_rel(rel2) ||
850-
restriction_is_constant_false(restrictlist, false))
851+
restriction_is_constant_false(restrictlist, joinrel, false))
851852
{
852853
mark_dummy_rel(joinrel);
853854
break;
@@ -870,7 +871,7 @@ populate_joinrel_with_paths(PlannerInfo *root, RelOptInfo *rel1,
870871
sjinfo) != NULL)
871872
{
872873
if (is_dummy_rel(rel1) || is_dummy_rel(rel2) ||
873-
restriction_is_constant_false(restrictlist, false))
874+
restriction_is_constant_false(restrictlist, joinrel, false))
874875
{
875876
mark_dummy_rel(joinrel);
876877
break;
@@ -885,12 +886,12 @@ populate_joinrel_with_paths(PlannerInfo *root, RelOptInfo *rel1,
885886
break;
886887
case JOIN_ANTI:
887888
if (is_dummy_rel(rel1) ||
888-
restriction_is_constant_false(restrictlist, true))
889+
restriction_is_constant_false(restrictlist, joinrel, true))
889890
{
890891
mark_dummy_rel(joinrel);
891892
break;
892893
}
893-
if (restriction_is_constant_false(restrictlist, false) &&
894+
if (restriction_is_constant_false(restrictlist, joinrel, false) &&
894895
bms_is_subset(rel2->relids, sjinfo->syn_righthand))
895896
mark_dummy_rel(rel2);
896897
add_paths_to_joinrel(root, joinrel, rel1, rel2,
@@ -1249,10 +1250,13 @@ mark_dummy_rel(RelOptInfo *rel)
12491250
* decide there's no match for an outer row, which is pretty stupid. So,
12501251
* we need to detect the case.
12511252
*
1252-
* If only_pushed_down is true, then consider only pushed-down quals.
1253+
* If only_pushed_down is true, then consider only quals that are pushed-down
1254+
* from the point of view of the joinrel.
12531255
*/
12541256
static bool
1255-
restriction_is_constant_false(List *restrictlist, bool only_pushed_down)
1257+
restriction_is_constant_false(List *restrictlist,
1258+
RelOptInfo *joinrel,
1259+
bool only_pushed_down)
12561260
{
12571261
ListCell *lc;
12581262

@@ -1266,7 +1270,7 @@ restriction_is_constant_false(List *restrictlist, bool only_pushed_down)
12661270
{
12671271
RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc);
12681272

1269-
if (only_pushed_down && !rinfo->is_pushed_down)
1273+
if (only_pushed_down && !RINFO_IS_PUSHED_DOWN(rinfo, joinrel->relids))
12701274
continue;
12711275

12721276
if (rinfo->clause && IsA(rinfo->clause, Const))
@@ -1411,8 +1415,9 @@ try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
14111415
* partition keys from given relations being joined.
14121416
*/
14131417
bool
1414-
have_partkey_equi_join(RelOptInfo *rel1, RelOptInfo *rel2, JoinType jointype,
1415-
List *restrictlist)
1418+
have_partkey_equi_join(RelOptInfo *joinrel,
1419+
RelOptInfo *rel1, RelOptInfo *rel2,
1420+
JoinType jointype, List *restrictlist)
14161421
{
14171422
PartitionScheme part_scheme = rel1->part_scheme;
14181423
ListCell *lc;
@@ -1438,7 +1443,8 @@ have_partkey_equi_join(RelOptInfo *rel1, RelOptInfo *rel2, JoinType jointype,
14381443
int ipk2;
14391444

14401445
/* If processing an outer join, only use its own join clauses. */
1441-
if (IS_OUTER_JOIN(jointype) && rinfo->is_pushed_down)
1446+
if (IS_OUTER_JOIN(jointype) &&
1447+
RINFO_IS_PUSHED_DOWN(rinfo, joinrel->relids))
14421448
continue;
14431449

14441450
/* Skip clauses which can not be used for a join. */

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

+5-5
Original file line numberDiff line numberDiff line change
@@ -253,8 +253,7 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
253253
* above the outer join, even if it references no other rels (it might
254254
* be from WHERE, for example).
255255
*/
256-
if (restrictinfo->is_pushed_down ||
257-
!bms_equal(restrictinfo->required_relids, joinrelids))
256+
if (RINFO_IS_PUSHED_DOWN(restrictinfo, joinrelids))
258257
{
259258
/*
260259
* If such a clause actually references the inner rel then join
@@ -422,8 +421,7 @@ remove_rel_from_query(PlannerInfo *root, int relid, Relids joinrelids)
422421

423422
remove_join_clause_from_rels(root, rinfo, rinfo->required_relids);
424423

425-
if (rinfo->is_pushed_down ||
426-
!bms_equal(rinfo->required_relids, joinrelids))
424+
if (RINFO_IS_PUSHED_DOWN(rinfo, joinrelids))
427425
{
428426
/* Recheck that qual doesn't actually reference the target rel */
429427
Assert(!bms_is_member(relid, rinfo->clause_relids));
@@ -1080,6 +1078,7 @@ is_innerrel_unique_for(PlannerInfo *root,
10801078
JoinType jointype,
10811079
List *restrictlist)
10821080
{
1081+
Relids joinrelids = bms_union(outerrelids, innerrel->relids);
10831082
List *clause_list = NIL;
10841083
ListCell *lc;
10851084

@@ -1098,7 +1097,8 @@ is_innerrel_unique_for(PlannerInfo *root,
10981097
* As noted above, if it's a pushed-down clause and we're at an outer
10991098
* join, we can't use it.
11001099
*/
1101-
if (restrictinfo->is_pushed_down && IS_OUTER_JOIN(jointype))
1100+
if (IS_OUTER_JOIN(jointype) &&
1101+
RINFO_IS_PUSHED_DOWN(restrictinfo, joinrelids))
11021102
continue;
11031103

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

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

+5
Original file line numberDiff line numberDiff line change
@@ -1775,6 +1775,11 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause,
17751775
* attach quals to the lowest level where they can be evaluated. But
17761776
* if we were ever to re-introduce a mechanism for delaying evaluation
17771777
* of "expensive" quals, this area would need work.
1778+
*
1779+
* Note: generally, use of is_pushed_down has to go through the macro
1780+
* RINFO_IS_PUSHED_DOWN, because that flag alone is not always sufficient
1781+
* to tell whether a clause must be treated as pushed-down in context.
1782+
* This seems like another reason why it should perhaps be rethought.
17781783
*----------
17791784
*/
17801785
if (is_deduced)

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

+2-1
Original file line numberDiff line numberDiff line change
@@ -1629,7 +1629,8 @@ build_joinrel_partition_info(RelOptInfo *joinrel, RelOptInfo *outer_rel,
16291629
*/
16301630
if (!IS_PARTITIONED_REL(outer_rel) || !IS_PARTITIONED_REL(inner_rel) ||
16311631
outer_rel->part_scheme != inner_rel->part_scheme ||
1632-
!have_partkey_equi_join(outer_rel, inner_rel, jointype, restrictlist))
1632+
!have_partkey_equi_join(joinrel, outer_rel, inner_rel,
1633+
jointype, restrictlist))
16331634
{
16341635
Assert(!IS_PARTITIONED_REL(joinrel));
16351636
return;

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

+2-10
Original file line numberDiff line numberDiff line change
@@ -373,7 +373,7 @@ extract_actual_clauses(List *restrictinfo_list,
373373
* extract_actual_join_clauses
374374
*
375375
* Extract bare clauses from 'restrictinfo_list', separating those that
376-
* syntactically match the join level from those that were pushed down.
376+
* semantically match the join level from those that were pushed down.
377377
* Pseudoconstant clauses are excluded from the results.
378378
*
379379
* This is only used at outer joins, since for plain joins we don't care
@@ -394,15 +394,7 @@ extract_actual_join_clauses(List *restrictinfo_list,
394394
{
395395
RestrictInfo *rinfo = lfirst_node(RestrictInfo, l);
396396

397-
/*
398-
* We must check both is_pushed_down and required_relids, since an
399-
* outer-join clause that's been pushed down to some lower join level
400-
* via path parameterization will not be marked is_pushed_down;
401-
* nonetheless, it must be treated as a filter clause not a join
402-
* clause so far as the lower join level is concerned.
403-
*/
404-
if (rinfo->is_pushed_down ||
405-
!bms_is_subset(rinfo->required_relids, joinrelids))
397+
if (RINFO_IS_PUSHED_DOWN(rinfo, joinrelids))
406398
{
407399
if (!rinfo->pseudoconstant)
408400
*otherquals = lappend(*otherquals, rinfo->clause);

‎src/include/nodes/relation.h

+16-1
Original file line numberDiff line numberDiff line change
@@ -1789,7 +1789,8 @@ typedef struct LimitPath
17891789
* if we decide that it can be pushed down into the nullable side of the join.
17901790
* In that case it acts as a plain filter qual for wherever it gets evaluated.
17911791
* (In short, is_pushed_down is only false for non-degenerate outer join
1792-
* conditions. Possibly we should rename it to reflect that meaning?)
1792+
* conditions. Possibly we should rename it to reflect that meaning? But
1793+
* see also the comments for RINFO_IS_PUSHED_DOWN, below.)
17931794
*
17941795
* RestrictInfo nodes also contain an outerjoin_delayed flag, which is true
17951796
* if the clause's applicability must be delayed due to any outer joins
@@ -1931,6 +1932,20 @@ typedef struct RestrictInfo
19311932
Selectivity right_mcvfreq; /* right side's most common val's freq */
19321933
} RestrictInfo;
19331934

1935+
/*
1936+
* This macro embodies the correct way to test whether a RestrictInfo is
1937+
* "pushed down" to a given outer join, that is, should be treated as a filter
1938+
* clause rather than a join clause at that outer join. This is certainly so
1939+
* if is_pushed_down is true; but examining that is not sufficient anymore,
1940+
* because outer-join clauses will get pushed down to lower outer joins when
1941+
* we generate a path for the lower outer join that is parameterized by the
1942+
* LHS of the upper one. We can detect such a clause by noting that its
1943+
* required_relids exceed the scope of the join.
1944+
*/
1945+
#define RINFO_IS_PUSHED_DOWN(rinfo, joinrelids) \
1946+
((rinfo)->is_pushed_down || \
1947+
!bms_is_subset((rinfo)->required_relids, joinrelids))
1948+
19341949
/*
19351950
* Since mergejoinscansel() is a relatively expensive function, and would
19361951
* otherwise be invoked many times while planning a large join tree,

‎src/include/optimizer/paths.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,8 @@ extern bool have_join_order_restriction(PlannerInfo *root,
115115
extern bool have_dangerous_phv(PlannerInfo *root,
116116
Relids outer_relids, Relids inner_params);
117117
extern void mark_dummy_rel(RelOptInfo *rel);
118-
extern bool have_partkey_equi_join(RelOptInfo *rel1, RelOptInfo *rel2,
118+
extern bool have_partkey_equi_join(RelOptInfo *joinrel,
119+
RelOptInfo *rel1, RelOptInfo *rel2,
119120
JoinType jointype, List *restrictlist);
120121

121122
/*

0 commit comments

Comments
 (0)