Skip to content

Commit 73fc2e5

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 6671e81 commit 73fc2e5

File tree

25 files changed

+358
-117
lines changed

25 files changed

+358
-117
lines changed

‎contrib/postgres_fdw/postgres_fdw.c

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,9 @@
4545
#include "utils/sampling.h"
4646
#include "utils/selfuncs.h"
4747

48+
/* source-code-compatibility hacks for pull_varnos() API change */
49+
#define make_restrictinfo(a,b,c,d,e,f,g,h,i) make_restrictinfo_new(a,b,c,d,e,f,g,h,i)
50+
4851
PG_MODULE_MAGIC;
4952

5053
/* Default CPU cost to start up a foreign query. */
@@ -5716,7 +5719,8 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel,
57165719
* RestrictInfos, so we must make our own.
57175720
*/
57185721
Assert(!IsA(expr, RestrictInfo));
5719-
rinfo = make_restrictinfo(expr,
5722+
rinfo = make_restrictinfo(root,
5723+
expr,
57205724
true,
57215725
false,
57225726
false,

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

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@
2626
#include "utils/lsyscache.h"
2727
#include "utils/selfuncs.h"
2828

29+
/* source-code-compatibility hacks for pull_varnos() API change */
30+
#define NumRelids(a,b) NumRelids_new(a,b)
31+
2932
/*
3033
* Data structure for accumulating info about possible range-query
3134
* clause pairs in clauselist_selectivity.
@@ -235,7 +238,7 @@ clauselist_selectivity_simple(PlannerInfo *root,
235238
}
236239
else
237240
{
238-
ok = (NumRelids(clause) == 1) &&
241+
ok = (NumRelids(root, clause) == 1) &&
239242
(is_pseudo_constant_clause(lsecond(expr->args)) ||
240243
(varonleft = false,
241244
is_pseudo_constant_clause(linitial(expr->args))));
@@ -519,7 +522,7 @@ bms_is_subset_singleton(const Bitmapset *s, int x)
519522
* restriction or join estimator. Subroutine for clause_selectivity().
520523
*/
521524
static inline bool
522-
treat_as_join_clause(Node *clause, RestrictInfo *rinfo,
525+
treat_as_join_clause(PlannerInfo *root, Node *clause, RestrictInfo *rinfo,
523526
int varRelid, SpecialJoinInfo *sjinfo)
524527
{
525528
if (varRelid != 0)
@@ -553,7 +556,7 @@ treat_as_join_clause(Node *clause, RestrictInfo *rinfo,
553556
if (rinfo)
554557
return (bms_membership(rinfo->clause_relids) == BMS_MULTIPLE);
555558
else
556-
return (NumRelids(clause) > 1);
559+
return (NumRelids(root, clause) > 1);
557560
}
558561
}
559562

@@ -759,7 +762,7 @@ clause_selectivity(PlannerInfo *root,
759762
OpExpr *opclause = (OpExpr *) clause;
760763
Oid opno = opclause->opno;
761764

762-
if (treat_as_join_clause(clause, rinfo, varRelid, sjinfo))
765+
if (treat_as_join_clause(root, clause, rinfo, varRelid, sjinfo))
763766
{
764767
/* Estimate selectivity for a join clause. */
765768
s1 = join_selectivity(root, opno,
@@ -795,7 +798,7 @@ clause_selectivity(PlannerInfo *root,
795798
funcclause->funcid,
796799
funcclause->args,
797800
funcclause->inputcollid,
798-
treat_as_join_clause(clause, rinfo,
801+
treat_as_join_clause(root, clause, rinfo,
799802
varRelid, sjinfo),
800803
varRelid,
801804
jointype,
@@ -806,7 +809,7 @@ clause_selectivity(PlannerInfo *root,
806809
/* Use node specific selectivity calculation function */
807810
s1 = scalararraysel(root,
808811
(ScalarArrayOpExpr *) clause,
809-
treat_as_join_clause(clause, rinfo,
812+
treat_as_join_clause(root, clause, rinfo,
810813
varRelid, sjinfo),
811814
varRelid,
812815
jointype,

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,9 @@
9898
#include "utils/tuplesort.h"
9999

100100

101+
/* source-code-compatibility hacks for pull_varnos() API change */
102+
#define pull_varnos(a,b) pull_varnos_new(a,b)
103+
101104
#define LOG2(x) (log(x) / 0.693147180559945)
102105

103106
/*
@@ -1848,7 +1851,7 @@ cost_incremental_sort(Path *path,
18481851
* Check if the expression contains Var with "varno 0" so that we
18491852
* don't call estimate_num_groups in that case.
18501853
*/
1851-
if (bms_is_member(0, pull_varnos((Node *) member->em_expr)))
1854+
if (bms_is_member(0, pull_varnos(root, (Node *) member->em_expr)))
18521855
{
18531856
unknown_varno = true;
18541857
break;

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

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,10 @@
3232
#include "utils/lsyscache.h"
3333

3434

35+
/* source-code-compatibility hacks for pull_varnos() API change */
36+
#define pull_varnos(a,b) pull_varnos_new(a,b)
37+
#define make_restrictinfo(a,b,c,d,e,f,g,h,i) make_restrictinfo_new(a,b,c,d,e,f,g,h,i)
38+
3539
static EquivalenceMember *add_eq_member(EquivalenceClass *ec,
3640
Expr *expr, Relids relids, Relids nullable_relids,
3741
bool is_child, Oid datatype);
@@ -195,7 +199,8 @@ process_equivalence(PlannerInfo *root,
195199
ntest->location = -1;
196200

197201
*p_restrictinfo =
198-
make_restrictinfo((Expr *) ntest,
202+
make_restrictinfo(root,
203+
(Expr *) ntest,
199204
restrictinfo->is_pushed_down,
200205
restrictinfo->outerjoin_delayed,
201206
restrictinfo->pseudoconstant,
@@ -713,7 +718,7 @@ get_eclass_for_sort_expr(PlannerInfo *root,
713718
/*
714719
* Get the precise set of nullable relids appearing in the expression.
715720
*/
716-
expr_relids = pull_varnos((Node *) expr);
721+
expr_relids = pull_varnos(root, (Node *) expr);
717722
nullable_relids = bms_intersect(nullable_relids, expr_relids);
718723

719724
newem = add_eq_member(newec, copyObject(expr), expr_relids,
@@ -1661,7 +1666,8 @@ create_join_clause(PlannerInfo *root,
16611666
*/
16621667
oldcontext = MemoryContextSwitchTo(root->planner_cxt);
16631668

1664-
rinfo = build_implied_join_equality(opno,
1669+
rinfo = build_implied_join_equality(root,
1670+
opno,
16651671
ec->ec_collation,
16661672
leftem->em_expr,
16671673
rightem->em_expr,
@@ -1961,7 +1967,8 @@ reconsider_outer_join_clause(PlannerInfo *root, RestrictInfo *rinfo,
19611967
cur_em->em_datatype);
19621968
if (!OidIsValid(eq_op))
19631969
continue; /* can't generate equality */
1964-
newrinfo = build_implied_join_equality(eq_op,
1970+
newrinfo = build_implied_join_equality(root,
1971+
eq_op,
19651972
cur_ec->ec_collation,
19661973
innervar,
19671974
cur_em->em_expr,
@@ -2104,7 +2111,8 @@ reconsider_full_join_clause(PlannerInfo *root, RestrictInfo *rinfo)
21042111
cur_em->em_datatype);
21052112
if (OidIsValid(eq_op))
21062113
{
2107-
newrinfo = build_implied_join_equality(eq_op,
2114+
newrinfo = build_implied_join_equality(root,
2115+
eq_op,
21082116
cur_ec->ec_collation,
21092117
leftvar,
21102118
cur_em->em_expr,
@@ -2119,7 +2127,8 @@ reconsider_full_join_clause(PlannerInfo *root, RestrictInfo *rinfo)
21192127
cur_em->em_datatype);
21202128
if (OidIsValid(eq_op))
21212129
{
2122-
newrinfo = build_implied_join_equality(eq_op,
2130+
newrinfo = build_implied_join_equality(root,
2131+
eq_op,
21232132
cur_ec->ec_collation,
21242133
rightvar,
21252134
cur_em->em_expr,

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

Lines changed: 48 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,12 @@
3636
#include "utils/selfuncs.h"
3737

3838

39+
/* source-code-compatibility hacks for pull_varnos() API change */
40+
#define pull_varnos(a,b) pull_varnos_new(a,b)
41+
#undef make_simple_restrictinfo
42+
#define make_simple_restrictinfo(root, clause) \
43+
make_restrictinfo_new(root, clause, true, false, false, 0, NULL, NULL, NULL)
44+
3945
/* XXX see PartCollMatchesExprColl */
4046
#define IndexCollMatchesExprColl(idxcollation, exprcollation) \
4147
((idxcollation) == InvalidOid || (idxcollation) == (exprcollation))
@@ -153,7 +159,8 @@ static IndexClause *match_clause_to_indexcol(PlannerInfo *root,
153159
RestrictInfo *rinfo,
154160
int indexcol,
155161
IndexOptInfo *index);
156-
static IndexClause *match_boolean_index_clause(RestrictInfo *rinfo,
162+
static IndexClause *match_boolean_index_clause(PlannerInfo *root,
163+
RestrictInfo *rinfo,
157164
int indexcol, IndexOptInfo *index);
158165
static IndexClause *match_opclause_to_indexcol(PlannerInfo *root,
159166
RestrictInfo *rinfo,
@@ -169,13 +176,16 @@ static IndexClause *get_index_clause_from_support(PlannerInfo *root,
169176
int indexarg,
170177
int indexcol,
171178
IndexOptInfo *index);
172-
static IndexClause *match_saopclause_to_indexcol(RestrictInfo *rinfo,
179+
static IndexClause *match_saopclause_to_indexcol(PlannerInfo *root,
180+
RestrictInfo *rinfo,
173181
int indexcol,
174182
IndexOptInfo *index);
175-
static IndexClause *match_rowcompare_to_indexcol(RestrictInfo *rinfo,
183+
static IndexClause *match_rowcompare_to_indexcol(PlannerInfo *root,
184+
RestrictInfo *rinfo,
176185
int indexcol,
177186
IndexOptInfo *index);
178-
static IndexClause *expand_indexqual_rowcompare(RestrictInfo *rinfo,
187+
static IndexClause *expand_indexqual_rowcompare(PlannerInfo *root,
188+
RestrictInfo *rinfo,
179189
int indexcol,
180190
IndexOptInfo *index,
181191
Oid expr_op,
@@ -2305,7 +2315,7 @@ match_clause_to_indexcol(PlannerInfo *root,
23052315
opfamily = index->opfamily[indexcol];
23062316
if (IsBooleanOpfamily(opfamily))
23072317
{
2308-
iclause = match_boolean_index_clause(rinfo, indexcol, index);
2318+
iclause = match_boolean_index_clause(root, rinfo, indexcol, index);
23092319
if (iclause)
23102320
return iclause;
23112321
}
@@ -2325,11 +2335,11 @@ match_clause_to_indexcol(PlannerInfo *root,
23252335
}
23262336
else if (IsA(clause, ScalarArrayOpExpr))
23272337
{
2328-
return match_saopclause_to_indexcol(rinfo, indexcol, index);
2338+
return match_saopclause_to_indexcol(root, rinfo, indexcol, index);
23292339
}
23302340
else if (IsA(clause, RowCompareExpr))
23312341
{
2332-
return match_rowcompare_to_indexcol(rinfo, indexcol, index);
2342+
return match_rowcompare_to_indexcol(root, rinfo, indexcol, index);
23332343
}
23342344
else if (index->amsearchnulls && IsA(clause, NullTest))
23352345
{
@@ -2368,7 +2378,8 @@ match_clause_to_indexcol(PlannerInfo *root,
23682378
* index's key, and if so, build a suitable IndexClause.
23692379
*/
23702380
static IndexClause *
2371-
match_boolean_index_clause(RestrictInfo *rinfo,
2381+
match_boolean_index_clause(PlannerInfo *root,
2382+
RestrictInfo *rinfo,
23722383
int indexcol,
23732384
IndexOptInfo *index)
23742385
{
@@ -2438,7 +2449,7 @@ match_boolean_index_clause(RestrictInfo *rinfo,
24382449
IndexClause *iclause = makeNode(IndexClause);
24392450

24402451
iclause->rinfo = rinfo;
2441-
iclause->indexquals = list_make1(make_simple_restrictinfo(op));
2452+
iclause->indexquals = list_make1(make_simple_restrictinfo(root, op));
24422453
iclause->lossy = false;
24432454
iclause->indexcol = indexcol;
24442455
iclause->indexcols = NIL;
@@ -2663,7 +2674,8 @@ get_index_clause_from_support(PlannerInfo *root,
26632674
{
26642675
Expr *clause = (Expr *) lfirst(lc);
26652676

2666-
indexquals = lappend(indexquals, make_simple_restrictinfo(clause));
2677+
indexquals = lappend(indexquals,
2678+
make_simple_restrictinfo(root, clause));
26672679
}
26682680

26692681
iclause->rinfo = rinfo;
@@ -2684,7 +2696,8 @@ get_index_clause_from_support(PlannerInfo *root,
26842696
* which see for comments.
26852697
*/
26862698
static IndexClause *
2687-
match_saopclause_to_indexcol(RestrictInfo *rinfo,
2699+
match_saopclause_to_indexcol(PlannerInfo *root,
2700+
RestrictInfo *rinfo,
26882701
int indexcol,
26892702
IndexOptInfo *index)
26902703
{
@@ -2703,7 +2716,7 @@ match_saopclause_to_indexcol(RestrictInfo *rinfo,
27032716
return NULL;
27042717
leftop = (Node *) linitial(saop->args);
27052718
rightop = (Node *) lsecond(saop->args);
2706-
right_relids = pull_varnos(rightop);
2719+
right_relids = pull_varnos(root, rightop);
27072720
expr_op = saop->opno;
27082721
expr_coll = saop->inputcollid;
27092722

@@ -2751,7 +2764,8 @@ match_saopclause_to_indexcol(RestrictInfo *rinfo,
27512764
* is handled by expand_indexqual_rowcompare().
27522765
*/
27532766
static IndexClause *
2754-
match_rowcompare_to_indexcol(RestrictInfo *rinfo,
2767+
match_rowcompare_to_indexcol(PlannerInfo *root,
2768+
RestrictInfo *rinfo,
27552769
int indexcol,
27562770
IndexOptInfo *index)
27572771
{
@@ -2796,14 +2810,14 @@ match_rowcompare_to_indexcol(RestrictInfo *rinfo,
27962810
* These syntactic tests are the same as in match_opclause_to_indexcol()
27972811
*/
27982812
if (match_index_to_operand(leftop, indexcol, index) &&
2799-
!bms_is_member(index_relid, pull_varnos(rightop)) &&
2813+
!bms_is_member(index_relid, pull_varnos(root, rightop)) &&
28002814
!contain_volatile_functions(rightop))
28012815
{
28022816
/* OK, indexkey is on left */
28032817
var_on_left = true;
28042818
}
28052819
else if (match_index_to_operand(rightop, indexcol, index) &&
2806-
!bms_is_member(index_relid, pull_varnos(leftop)) &&
2820+
!bms_is_member(index_relid, pull_varnos(root, leftop)) &&
28072821
!contain_volatile_functions(leftop))
28082822
{
28092823
/* indexkey is on right, so commute the operator */
@@ -2822,7 +2836,8 @@ match_rowcompare_to_indexcol(RestrictInfo *rinfo,
28222836
case BTLessEqualStrategyNumber:
28232837
case BTGreaterEqualStrategyNumber:
28242838
case BTGreaterStrategyNumber:
2825-
return expand_indexqual_rowcompare(rinfo,
2839+
return expand_indexqual_rowcompare(root,
2840+
rinfo,
28262841
indexcol,
28272842
index,
28282843
expr_op,
@@ -2856,7 +2871,8 @@ match_rowcompare_to_indexcol(RestrictInfo *rinfo,
28562871
* but we split it out for comprehensibility.
28572872
*/
28582873
static IndexClause *
2859-
expand_indexqual_rowcompare(RestrictInfo *rinfo,
2874+
expand_indexqual_rowcompare(PlannerInfo *root,
2875+
RestrictInfo *rinfo,
28602876
int indexcol,
28612877
IndexOptInfo *index,
28622878
Oid expr_op,
@@ -2926,7 +2942,7 @@ expand_indexqual_rowcompare(RestrictInfo *rinfo,
29262942
if (expr_op == InvalidOid)
29272943
break; /* operator is not usable */
29282944
}
2929-
if (bms_is_member(index->rel->relid, pull_varnos(constop)))
2945+
if (bms_is_member(index->rel->relid, pull_varnos(root, constop)))
29302946
break; /* no good, Var on wrong side */
29312947
if (contain_volatile_functions(constop))
29322948
break; /* no good, volatile comparison value */
@@ -3036,7 +3052,8 @@ expand_indexqual_rowcompare(RestrictInfo *rinfo,
30363052
matching_cols);
30373053
rc->rargs = list_truncate(copyObject(non_var_args),
30383054
matching_cols);
3039-
iclause->indexquals = list_make1(make_simple_restrictinfo((Expr *) rc));
3055+
iclause->indexquals = list_make1(make_simple_restrictinfo(root,
3056+
(Expr *) rc));
30403057
}
30413058
else
30423059
{
@@ -3050,7 +3067,7 @@ expand_indexqual_rowcompare(RestrictInfo *rinfo,
30503067
copyObject(linitial(non_var_args)),
30513068
InvalidOid,
30523069
linitial_oid(clause->inputcollids));
3053-
iclause->indexquals = list_make1(make_simple_restrictinfo(op));
3070+
iclause->indexquals = list_make1(make_simple_restrictinfo(root, op));
30543071
}
30553072
}
30563073

@@ -3667,7 +3684,9 @@ relation_has_unique_index_for(PlannerInfo *root, RelOptInfo *rel,
36673684
* specified index column matches a boolean restriction clause.
36683685
*/
36693686
bool
3670-
indexcol_is_bool_constant_for_query(IndexOptInfo *index, int indexcol)
3687+
indexcol_is_bool_constant_for_query(PlannerInfo *root,
3688+
IndexOptInfo *index,
3689+
int indexcol)
36713690
{
36723691
ListCell *lc;
36733692

@@ -3689,7 +3708,7 @@ indexcol_is_bool_constant_for_query(IndexOptInfo *index, int indexcol)
36893708
continue;
36903709

36913710
/* See if we can match the clause's expression to the index column */
3692-
if (match_boolean_index_clause(rinfo, indexcol, index))
3711+
if (match_boolean_index_clause(root, rinfo, indexcol, index))
36933712
return true;
36943713
}
36953714

@@ -3802,9 +3821,15 @@ match_index_to_operand(Node *operand,
38023821
*/
38033822
bool
38043823
is_pseudo_constant_for_index(Node *expr, IndexOptInfo *index)
3824+
{
3825+
return is_pseudo_constant_for_index_new(NULL, expr, index);
3826+
}
3827+
3828+
bool
3829+
is_pseudo_constant_for_index_new(PlannerInfo *root, Node *expr, IndexOptInfo *index)
38053830
{
38063831
/* pull_varnos is cheaper than volatility check, so do that first */
3807-
if (bms_is_member(index->rel->relid, pull_varnos(expr)))
3832+
if (bms_is_member(index->rel->relid, pull_varnos(root, expr)))
38083833
return false; /* no good, contains Var of table */
38093834
if (contain_volatile_functions(expr))
38103835
return false; /* no good, volatile comparison value */

0 commit comments

Comments
 (0)