Skip to content

Commit 55dc86e

Browse files
tglsfdctglsfdc
committed
Fix pull_varnos' miscomputation of relids set for a PlaceHolderVar.
Previously, pull_varnos() took the relids of a PlaceHolderVar as being equal to the relids in its contents, but that fails to account for the possibility that we have to postpone evaluation of the PHV due to outer joins. This could result in a malformed plan. The known cases end up triggering the "failed to assign all NestLoopParams to plan nodes" sanity check in createplan.c, but other symptoms may be possible. The right value to use is the join level we actually intend to evaluate the PHV at. We can get that from the ph_eval_at field of the associated PlaceHolderInfo. However, there are some places that call pull_varnos() before the PlaceHolderInfos have been created; in that case, fall back to the conservative assumption that the PHV will be evaluated at its syntactic level. (In principle this might result in missing some legal optimization, but I'm not aware of any cases where it's an issue in practice.) Things are also a bit ticklish for calls occurring during deconstruct_jointree(), but AFAICS the ph_eval_at fields should have reached their final values by the time we need them. The main problem in making this work is that pull_varnos() has no way to get at the PlaceHolderInfos. We can fix that easily, if a bit tediously, in HEAD by passing it the planner "root" pointer. In the back branches that'd cause an unacceptable API/ABI break for extensions, so leave the existing entry points alone and add new ones with the additional parameter. (If an old entry point is called and encounters a PHV, it'll fall back to using the syntactic level, again possibly missing some valid optimization.) Back-patch to v12. The computation is surely also wrong before that, but it appears that we cannot reach a bad plan thanks to join order restrictions imposed on the subquery that the PlaceHolderVar came from. The error only became reachable when commit 4be058f allowed trivial subqueries to be collapsed out completely, eliminating their join order restrictions. Per report from Stephan Springl. Discussion: https://postgr.es/m/171041.1610849523@sss.pgh.pa.us
1 parent 920f853 commit 55dc86e

25 files changed

+261
-131
lines changed

‎contrib/postgres_fdw/postgres_fdw.c

+2-1
Original file line numberDiff line numberDiff line change
@@ -5882,7 +5882,8 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel,
58825882
* RestrictInfos, so we must make our own.
58835883
*/
58845884
Assert(!IsA(expr, RestrictInfo));
5885-
rinfo = make_restrictinfo(expr,
5885+
rinfo = make_restrictinfo(root,
5886+
expr,
58865887
true,
58875888
false,
58885889
false,

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

+6-6
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ clauselist_selectivity_ext(PlannerInfo *root,
227227
}
228228
else
229229
{
230-
ok = (NumRelids(clause) == 1) &&
230+
ok = (NumRelids(root, clause) == 1) &&
231231
(is_pseudo_constant_clause(lsecond(expr->args)) ||
232232
(varonleft = false,
233233
is_pseudo_constant_clause(linitial(expr->args))));
@@ -609,7 +609,7 @@ bms_is_subset_singleton(const Bitmapset *s, int x)
609609
* restriction or join estimator. Subroutine for clause_selectivity().
610610
*/
611611
static inline bool
612-
treat_as_join_clause(Node *clause, RestrictInfo *rinfo,
612+
treat_as_join_clause(PlannerInfo *root, Node *clause, RestrictInfo *rinfo,
613613
int varRelid, SpecialJoinInfo *sjinfo)
614614
{
615615
if (varRelid != 0)
@@ -643,7 +643,7 @@ treat_as_join_clause(Node *clause, RestrictInfo *rinfo,
643643
if (rinfo)
644644
return (bms_membership(rinfo->clause_relids) == BMS_MULTIPLE);
645645
else
646-
return (NumRelids(clause) > 1);
646+
return (NumRelids(root, clause) > 1);
647647
}
648648
}
649649

@@ -860,7 +860,7 @@ clause_selectivity_ext(PlannerInfo *root,
860860
OpExpr *opclause = (OpExpr *) clause;
861861
Oid opno = opclause->opno;
862862

863-
if (treat_as_join_clause(clause, rinfo, varRelid, sjinfo))
863+
if (treat_as_join_clause(root, clause, rinfo, varRelid, sjinfo))
864864
{
865865
/* Estimate selectivity for a join clause. */
866866
s1 = join_selectivity(root, opno,
@@ -896,7 +896,7 @@ clause_selectivity_ext(PlannerInfo *root,
896896
funcclause->funcid,
897897
funcclause->args,
898898
funcclause->inputcollid,
899-
treat_as_join_clause(clause, rinfo,
899+
treat_as_join_clause(root, clause, rinfo,
900900
varRelid, sjinfo),
901901
varRelid,
902902
jointype,
@@ -907,7 +907,7 @@ clause_selectivity_ext(PlannerInfo *root,
907907
/* Use node specific selectivity calculation function */
908908
s1 = scalararraysel(root,
909909
(ScalarArrayOpExpr *) clause,
910-
treat_as_join_clause(clause, rinfo,
910+
treat_as_join_clause(root, clause, rinfo,
911911
varRelid, sjinfo),
912912
varRelid,
913913
jointype,

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -1858,7 +1858,7 @@ cost_incremental_sort(Path *path,
18581858
* Check if the expression contains Var with "varno 0" so that we
18591859
* don't call estimate_num_groups in that case.
18601860
*/
1861-
if (bms_is_member(0, pull_varnos((Node *) member->em_expr)))
1861+
if (bms_is_member(0, pull_varnos(root, (Node *) member->em_expr)))
18621862
{
18631863
unknown_varno = true;
18641864
break;

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

+11-6
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,8 @@ process_equivalence(PlannerInfo *root,
196196
ntest->location = -1;
197197

198198
*p_restrictinfo =
199-
make_restrictinfo((Expr *) ntest,
199+
make_restrictinfo(root,
200+
(Expr *) ntest,
200201
restrictinfo->is_pushed_down,
201202
restrictinfo->outerjoin_delayed,
202203
restrictinfo->pseudoconstant,
@@ -716,7 +717,7 @@ get_eclass_for_sort_expr(PlannerInfo *root,
716717
/*
717718
* Get the precise set of nullable relids appearing in the expression.
718719
*/
719-
expr_relids = pull_varnos((Node *) expr);
720+
expr_relids = pull_varnos(root, (Node *) expr);
720721
nullable_relids = bms_intersect(nullable_relids, expr_relids);
721722

722723
newem = add_eq_member(newec, copyObject(expr), expr_relids,
@@ -1696,7 +1697,8 @@ create_join_clause(PlannerInfo *root,
16961697
*/
16971698
oldcontext = MemoryContextSwitchTo(root->planner_cxt);
16981699

1699-
rinfo = build_implied_join_equality(opno,
1700+
rinfo = build_implied_join_equality(root,
1701+
opno,
17001702
ec->ec_collation,
17011703
leftem->em_expr,
17021704
rightem->em_expr,
@@ -1996,7 +1998,8 @@ reconsider_outer_join_clause(PlannerInfo *root, RestrictInfo *rinfo,
19961998
cur_em->em_datatype);
19971999
if (!OidIsValid(eq_op))
19982000
continue; /* can't generate equality */
1999-
newrinfo = build_implied_join_equality(eq_op,
2001+
newrinfo = build_implied_join_equality(root,
2002+
eq_op,
20002003
cur_ec->ec_collation,
20012004
innervar,
20022005
cur_em->em_expr,
@@ -2141,7 +2144,8 @@ reconsider_full_join_clause(PlannerInfo *root, RestrictInfo *rinfo)
21412144
cur_em->em_datatype);
21422145
if (OidIsValid(eq_op))
21432146
{
2144-
newrinfo = build_implied_join_equality(eq_op,
2147+
newrinfo = build_implied_join_equality(root,
2148+
eq_op,
21452149
cur_ec->ec_collation,
21462150
leftvar,
21472151
cur_em->em_expr,
@@ -2156,7 +2160,8 @@ reconsider_full_join_clause(PlannerInfo *root, RestrictInfo *rinfo)
21562160
cur_em->em_datatype);
21572161
if (OidIsValid(eq_op))
21582162
{
2159-
newrinfo = build_implied_join_equality(eq_op,
2163+
newrinfo = build_implied_join_equality(root,
2164+
eq_op,
21602165
cur_ec->ec_collation,
21612166
rightvar,
21622167
cur_em->em_expr,

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

+37-24
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,8 @@ static IndexClause *match_clause_to_indexcol(PlannerInfo *root,
153153
RestrictInfo *rinfo,
154154
int indexcol,
155155
IndexOptInfo *index);
156-
static IndexClause *match_boolean_index_clause(RestrictInfo *rinfo,
156+
static IndexClause *match_boolean_index_clause(PlannerInfo *root,
157+
RestrictInfo *rinfo,
157158
int indexcol, IndexOptInfo *index);
158159
static IndexClause *match_opclause_to_indexcol(PlannerInfo *root,
159160
RestrictInfo *rinfo,
@@ -169,13 +170,16 @@ static IndexClause *get_index_clause_from_support(PlannerInfo *root,
169170
int indexarg,
170171
int indexcol,
171172
IndexOptInfo *index);
172-
static IndexClause *match_saopclause_to_indexcol(RestrictInfo *rinfo,
173+
static IndexClause *match_saopclause_to_indexcol(PlannerInfo *root,
174+
RestrictInfo *rinfo,
173175
int indexcol,
174176
IndexOptInfo *index);
175-
static IndexClause *match_rowcompare_to_indexcol(RestrictInfo *rinfo,
177+
static IndexClause *match_rowcompare_to_indexcol(PlannerInfo *root,
178+
RestrictInfo *rinfo,
176179
int indexcol,
177180
IndexOptInfo *index);
178-
static IndexClause *expand_indexqual_rowcompare(RestrictInfo *rinfo,
181+
static IndexClause *expand_indexqual_rowcompare(PlannerInfo *root,
182+
RestrictInfo *rinfo,
179183
int indexcol,
180184
IndexOptInfo *index,
181185
Oid expr_op,
@@ -2305,7 +2309,7 @@ match_clause_to_indexcol(PlannerInfo *root,
23052309
opfamily = index->opfamily[indexcol];
23062310
if (IsBooleanOpfamily(opfamily))
23072311
{
2308-
iclause = match_boolean_index_clause(rinfo, indexcol, index);
2312+
iclause = match_boolean_index_clause(root, rinfo, indexcol, index);
23092313
if (iclause)
23102314
return iclause;
23112315
}
@@ -2325,11 +2329,11 @@ match_clause_to_indexcol(PlannerInfo *root,
23252329
}
23262330
else if (IsA(clause, ScalarArrayOpExpr))
23272331
{
2328-
return match_saopclause_to_indexcol(rinfo, indexcol, index);
2332+
return match_saopclause_to_indexcol(root, rinfo, indexcol, index);
23292333
}
23302334
else if (IsA(clause, RowCompareExpr))
23312335
{
2332-
return match_rowcompare_to_indexcol(rinfo, indexcol, index);
2336+
return match_rowcompare_to_indexcol(root, rinfo, indexcol, index);
23332337
}
23342338
else if (index->amsearchnulls && IsA(clause, NullTest))
23352339
{
@@ -2368,7 +2372,8 @@ match_clause_to_indexcol(PlannerInfo *root,
23682372
* index's key, and if so, build a suitable IndexClause.
23692373
*/
23702374
static IndexClause *
2371-
match_boolean_index_clause(RestrictInfo *rinfo,
2375+
match_boolean_index_clause(PlannerInfo *root,
2376+
RestrictInfo *rinfo,
23722377
int indexcol,
23732378
IndexOptInfo *index)
23742379
{
@@ -2438,7 +2443,7 @@ match_boolean_index_clause(RestrictInfo *rinfo,
24382443
IndexClause *iclause = makeNode(IndexClause);
24392444

24402445
iclause->rinfo = rinfo;
2441-
iclause->indexquals = list_make1(make_simple_restrictinfo(op));
2446+
iclause->indexquals = list_make1(make_simple_restrictinfo(root, op));
24422447
iclause->lossy = false;
24432448
iclause->indexcol = indexcol;
24442449
iclause->indexcols = NIL;
@@ -2663,7 +2668,8 @@ get_index_clause_from_support(PlannerInfo *root,
26632668
{
26642669
Expr *clause = (Expr *) lfirst(lc);
26652670

2666-
indexquals = lappend(indexquals, make_simple_restrictinfo(clause));
2671+
indexquals = lappend(indexquals,
2672+
make_simple_restrictinfo(root, clause));
26672673
}
26682674

26692675
iclause->rinfo = rinfo;
@@ -2684,7 +2690,8 @@ get_index_clause_from_support(PlannerInfo *root,
26842690
* which see for comments.
26852691
*/
26862692
static IndexClause *
2687-
match_saopclause_to_indexcol(RestrictInfo *rinfo,
2693+
match_saopclause_to_indexcol(PlannerInfo *root,
2694+
RestrictInfo *rinfo,
26882695
int indexcol,
26892696
IndexOptInfo *index)
26902697
{
@@ -2703,7 +2710,7 @@ match_saopclause_to_indexcol(RestrictInfo *rinfo,
27032710
return NULL;
27042711
leftop = (Node *) linitial(saop->args);
27052712
rightop = (Node *) lsecond(saop->args);
2706-
right_relids = pull_varnos(rightop);
2713+
right_relids = pull_varnos(root, rightop);
27072714
expr_op = saop->opno;
27082715
expr_coll = saop->inputcollid;
27092716

@@ -2751,7 +2758,8 @@ match_saopclause_to_indexcol(RestrictInfo *rinfo,
27512758
* is handled by expand_indexqual_rowcompare().
27522759
*/
27532760
static IndexClause *
2754-
match_rowcompare_to_indexcol(RestrictInfo *rinfo,
2761+
match_rowcompare_to_indexcol(PlannerInfo *root,
2762+
RestrictInfo *rinfo,
27552763
int indexcol,
27562764
IndexOptInfo *index)
27572765
{
@@ -2796,14 +2804,14 @@ match_rowcompare_to_indexcol(RestrictInfo *rinfo,
27962804
* These syntactic tests are the same as in match_opclause_to_indexcol()
27972805
*/
27982806
if (match_index_to_operand(leftop, indexcol, index) &&
2799-
!bms_is_member(index_relid, pull_varnos(rightop)) &&
2807+
!bms_is_member(index_relid, pull_varnos(root, rightop)) &&
28002808
!contain_volatile_functions(rightop))
28012809
{
28022810
/* OK, indexkey is on left */
28032811
var_on_left = true;
28042812
}
28052813
else if (match_index_to_operand(rightop, indexcol, index) &&
2806-
!bms_is_member(index_relid, pull_varnos(leftop)) &&
2814+
!bms_is_member(index_relid, pull_varnos(root, leftop)) &&
28072815
!contain_volatile_functions(leftop))
28082816
{
28092817
/* indexkey is on right, so commute the operator */
@@ -2822,7 +2830,8 @@ match_rowcompare_to_indexcol(RestrictInfo *rinfo,
28222830
case BTLessEqualStrategyNumber:
28232831
case BTGreaterEqualStrategyNumber:
28242832
case BTGreaterStrategyNumber:
2825-
return expand_indexqual_rowcompare(rinfo,
2833+
return expand_indexqual_rowcompare(root,
2834+
rinfo,
28262835
indexcol,
28272836
index,
28282837
expr_op,
@@ -2856,7 +2865,8 @@ match_rowcompare_to_indexcol(RestrictInfo *rinfo,
28562865
* but we split it out for comprehensibility.
28572866
*/
28582867
static IndexClause *
2859-
expand_indexqual_rowcompare(RestrictInfo *rinfo,
2868+
expand_indexqual_rowcompare(PlannerInfo *root,
2869+
RestrictInfo *rinfo,
28602870
int indexcol,
28612871
IndexOptInfo *index,
28622872
Oid expr_op,
@@ -2926,7 +2936,7 @@ expand_indexqual_rowcompare(RestrictInfo *rinfo,
29262936
if (expr_op == InvalidOid)
29272937
break; /* operator is not usable */
29282938
}
2929-
if (bms_is_member(index->rel->relid, pull_varnos(constop)))
2939+
if (bms_is_member(index->rel->relid, pull_varnos(root, constop)))
29302940
break; /* no good, Var on wrong side */
29312941
if (contain_volatile_functions(constop))
29322942
break; /* no good, volatile comparison value */
@@ -3036,7 +3046,8 @@ expand_indexqual_rowcompare(RestrictInfo *rinfo,
30363046
matching_cols);
30373047
rc->rargs = list_truncate(copyObject(non_var_args),
30383048
matching_cols);
3039-
iclause->indexquals = list_make1(make_simple_restrictinfo((Expr *) rc));
3049+
iclause->indexquals = list_make1(make_simple_restrictinfo(root,
3050+
(Expr *) rc));
30403051
}
30413052
else
30423053
{
@@ -3050,7 +3061,7 @@ expand_indexqual_rowcompare(RestrictInfo *rinfo,
30503061
copyObject(linitial(non_var_args)),
30513062
InvalidOid,
30523063
linitial_oid(clause->inputcollids));
3053-
iclause->indexquals = list_make1(make_simple_restrictinfo(op));
3064+
iclause->indexquals = list_make1(make_simple_restrictinfo(root, op));
30543065
}
30553066
}
30563067

@@ -3667,7 +3678,9 @@ relation_has_unique_index_for(PlannerInfo *root, RelOptInfo *rel,
36673678
* specified index column matches a boolean restriction clause.
36683679
*/
36693680
bool
3670-
indexcol_is_bool_constant_for_query(IndexOptInfo *index, int indexcol)
3681+
indexcol_is_bool_constant_for_query(PlannerInfo *root,
3682+
IndexOptInfo *index,
3683+
int indexcol)
36713684
{
36723685
ListCell *lc;
36733686

@@ -3689,7 +3702,7 @@ indexcol_is_bool_constant_for_query(IndexOptInfo *index, int indexcol)
36893702
continue;
36903703

36913704
/* See if we can match the clause's expression to the index column */
3692-
if (match_boolean_index_clause(rinfo, indexcol, index))
3705+
if (match_boolean_index_clause(root, rinfo, indexcol, index))
36933706
return true;
36943707
}
36953708

@@ -3801,10 +3814,10 @@ match_index_to_operand(Node *operand,
38013814
* index: the index of interest
38023815
*/
38033816
bool
3804-
is_pseudo_constant_for_index(Node *expr, IndexOptInfo *index)
3817+
is_pseudo_constant_for_index(PlannerInfo *root, Node *expr, IndexOptInfo *index)
38053818
{
38063819
/* pull_varnos is cheaper than volatility check, so do that first */
3807-
if (bms_is_member(index->rel->relid, pull_varnos(expr)))
3820+
if (bms_is_member(index->rel->relid, pull_varnos(root, expr)))
38083821
return false; /* no good, contains Var of table */
38093822
if (contain_volatile_functions(expr))
38103823
return false; /* no good, volatile comparison value */

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -598,7 +598,7 @@ build_index_pathkeys(PlannerInfo *root,
598598
* should stop considering index columns; any lower-order sort
599599
* keys won't be useful either.
600600
*/
601-
if (!indexcol_is_bool_constant_for_query(index, i))
601+
if (!indexcol_is_bool_constant_for_query(root, index, i))
602602
break;
603603
}
604604

0 commit comments

Comments
 (0)