Skip to content

Commit 1cce024

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 561dd8d commit 1cce024

File tree

24 files changed

+354
-116
lines changed

24 files changed

+354
-116
lines changed

‎contrib/postgres_fdw/postgres_fdw.c

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

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

4952
/* Default CPU cost to start up a foreign query. */
@@ -5665,7 +5668,8 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel,
56655668
* RestrictInfos, so we must make our own.
56665669
*/
56675670
Assert(!IsA(expr, RestrictInfo));
5668-
rinfo = make_restrictinfo(expr,
5671+
rinfo = make_restrictinfo(root,
5672+
expr,
56695673
true,
56705674
false,
56715675
false,

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

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@
2727
#include "statistics/statistics.h"
2828

2929

30+
/* source-code-compatibility hacks for pull_varnos() API change */
31+
#define NumRelids(a,b) NumRelids_new(a,b)
32+
3033
/*
3134
* Data structure for accumulating info about possible range-query
3235
* clause pairs in clauselist_selectivity.
@@ -236,7 +239,7 @@ clauselist_selectivity_simple(PlannerInfo *root,
236239
}
237240
else
238241
{
239-
ok = (NumRelids(clause) == 1) &&
242+
ok = (NumRelids(root, clause) == 1) &&
240243
(is_pseudo_constant_clause(lsecond(expr->args)) ||
241244
(varonleft = false,
242245
is_pseudo_constant_clause(linitial(expr->args))));
@@ -520,7 +523,7 @@ bms_is_subset_singleton(const Bitmapset *s, int x)
520523
* restriction or join estimator. Subroutine for clause_selectivity().
521524
*/
522525
static inline bool
523-
treat_as_join_clause(Node *clause, RestrictInfo *rinfo,
526+
treat_as_join_clause(PlannerInfo *root, Node *clause, RestrictInfo *rinfo,
524527
int varRelid, SpecialJoinInfo *sjinfo)
525528
{
526529
if (varRelid != 0)
@@ -554,7 +557,7 @@ treat_as_join_clause(Node *clause, RestrictInfo *rinfo,
554557
if (rinfo)
555558
return (bms_membership(rinfo->clause_relids) == BMS_MULTIPLE);
556559
else
557-
return (NumRelids(clause) > 1);
560+
return (NumRelids(root, clause) > 1);
558561
}
559562
}
560563

@@ -760,7 +763,7 @@ clause_selectivity(PlannerInfo *root,
760763
OpExpr *opclause = (OpExpr *) clause;
761764
Oid opno = opclause->opno;
762765

763-
if (treat_as_join_clause(clause, rinfo, varRelid, sjinfo))
766+
if (treat_as_join_clause(root, clause, rinfo, varRelid, sjinfo))
764767
{
765768
/* Estimate selectivity for a join clause. */
766769
s1 = join_selectivity(root, opno,
@@ -796,7 +799,7 @@ clause_selectivity(PlannerInfo *root,
796799
funcclause->funcid,
797800
funcclause->args,
798801
funcclause->inputcollid,
799-
treat_as_join_clause(clause, rinfo,
802+
treat_as_join_clause(root, clause, rinfo,
800803
varRelid, sjinfo),
801804
varRelid,
802805
jointype,
@@ -807,7 +810,7 @@ clause_selectivity(PlannerInfo *root,
807810
/* Use node specific selectivity calculation function */
808811
s1 = scalararraysel(root,
809812
(ScalarArrayOpExpr *) clause,
810-
treat_as_join_clause(clause, rinfo,
813+
treat_as_join_clause(root, clause, rinfo,
811814
varRelid, sjinfo),
812815
varRelid,
813816
jointype,

‎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);
@@ -191,7 +195,8 @@ process_equivalence(PlannerInfo *root,
191195
ntest->location = -1;
192196

193197
*p_restrictinfo =
194-
make_restrictinfo((Expr *) ntest,
198+
make_restrictinfo(root,
199+
(Expr *) ntest,
195200
restrictinfo->is_pushed_down,
196201
restrictinfo->outerjoin_delayed,
197202
restrictinfo->pseudoconstant,
@@ -708,7 +713,7 @@ get_eclass_for_sort_expr(PlannerInfo *root,
708713
/*
709714
* Get the precise set of nullable relids appearing in the expression.
710715
*/
711-
expr_relids = pull_varnos((Node *) expr);
716+
expr_relids = pull_varnos(root, (Node *) expr);
712717
nullable_relids = bms_intersect(nullable_relids, expr_relids);
713718

714719
newem = add_eq_member(newec, copyObject(expr), expr_relids,
@@ -1449,7 +1454,8 @@ create_join_clause(PlannerInfo *root,
14491454
*/
14501455
oldcontext = MemoryContextSwitchTo(root->planner_cxt);
14511456

1452-
rinfo = build_implied_join_equality(opno,
1457+
rinfo = build_implied_join_equality(root,
1458+
opno,
14531459
ec->ec_collation,
14541460
leftem->em_expr,
14551461
rightem->em_expr,
@@ -1763,7 +1769,8 @@ reconsider_outer_join_clause(PlannerInfo *root, RestrictInfo *rinfo,
17631769
cur_em->em_datatype);
17641770
if (!OidIsValid(eq_op))
17651771
continue; /* can't generate equality */
1766-
newrinfo = build_implied_join_equality(eq_op,
1772+
newrinfo = build_implied_join_equality(root,
1773+
eq_op,
17671774
cur_ec->ec_collation,
17681775
innervar,
17691776
cur_em->em_expr,
@@ -1906,7 +1913,8 @@ reconsider_full_join_clause(PlannerInfo *root, RestrictInfo *rinfo)
19061913
cur_em->em_datatype);
19071914
if (OidIsValid(eq_op))
19081915
{
1909-
newrinfo = build_implied_join_equality(eq_op,
1916+
newrinfo = build_implied_join_equality(root,
1917+
eq_op,
19101918
cur_ec->ec_collation,
19111919
leftvar,
19121920
cur_em->em_expr,
@@ -1921,7 +1929,8 @@ reconsider_full_join_clause(PlannerInfo *root, RestrictInfo *rinfo)
19211929
cur_em->em_datatype);
19221930
if (OidIsValid(eq_op))
19231931
{
1924-
newrinfo = build_implied_join_equality(eq_op,
1932+
newrinfo = build_implied_join_equality(root,
1933+
eq_op,
19251934
cur_ec->ec_collation,
19261935
rightvar,
19271936
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,
@@ -2313,7 +2323,7 @@ match_clause_to_indexcol(PlannerInfo *root,
23132323
opfamily = index->opfamily[indexcol];
23142324
if (IsBooleanOpfamily(opfamily))
23152325
{
2316-
iclause = match_boolean_index_clause(rinfo, indexcol, index);
2326+
iclause = match_boolean_index_clause(root, rinfo, indexcol, index);
23172327
if (iclause)
23182328
return iclause;
23192329
}
@@ -2333,11 +2343,11 @@ match_clause_to_indexcol(PlannerInfo *root,
23332343
}
23342344
else if (IsA(clause, ScalarArrayOpExpr))
23352345
{
2336-
return match_saopclause_to_indexcol(rinfo, indexcol, index);
2346+
return match_saopclause_to_indexcol(root, rinfo, indexcol, index);
23372347
}
23382348
else if (IsA(clause, RowCompareExpr))
23392349
{
2340-
return match_rowcompare_to_indexcol(rinfo, indexcol, index);
2350+
return match_rowcompare_to_indexcol(root, rinfo, indexcol, index);
23412351
}
23422352
else if (index->amsearchnulls && IsA(clause, NullTest))
23432353
{
@@ -2376,7 +2386,8 @@ match_clause_to_indexcol(PlannerInfo *root,
23762386
* index's key, and if so, build a suitable IndexClause.
23772387
*/
23782388
static IndexClause *
2379-
match_boolean_index_clause(RestrictInfo *rinfo,
2389+
match_boolean_index_clause(PlannerInfo *root,
2390+
RestrictInfo *rinfo,
23802391
int indexcol,
23812392
IndexOptInfo *index)
23822393
{
@@ -2446,7 +2457,7 @@ match_boolean_index_clause(RestrictInfo *rinfo,
24462457
IndexClause *iclause = makeNode(IndexClause);
24472458

24482459
iclause->rinfo = rinfo;
2449-
iclause->indexquals = list_make1(make_simple_restrictinfo(op));
2460+
iclause->indexquals = list_make1(make_simple_restrictinfo(root, op));
24502461
iclause->lossy = false;
24512462
iclause->indexcol = indexcol;
24522463
iclause->indexcols = NIL;
@@ -2671,7 +2682,8 @@ get_index_clause_from_support(PlannerInfo *root,
26712682
{
26722683
Expr *clause = (Expr *) lfirst(lc);
26732684

2674-
indexquals = lappend(indexquals, make_simple_restrictinfo(clause));
2685+
indexquals = lappend(indexquals,
2686+
make_simple_restrictinfo(root, clause));
26752687
}
26762688

26772689
iclause->rinfo = rinfo;
@@ -2692,7 +2704,8 @@ get_index_clause_from_support(PlannerInfo *root,
26922704
* which see for comments.
26932705
*/
26942706
static IndexClause *
2695-
match_saopclause_to_indexcol(RestrictInfo *rinfo,
2707+
match_saopclause_to_indexcol(PlannerInfo *root,
2708+
RestrictInfo *rinfo,
26962709
int indexcol,
26972710
IndexOptInfo *index)
26982711
{
@@ -2711,7 +2724,7 @@ match_saopclause_to_indexcol(RestrictInfo *rinfo,
27112724
return NULL;
27122725
leftop = (Node *) linitial(saop->args);
27132726
rightop = (Node *) lsecond(saop->args);
2714-
right_relids = pull_varnos(rightop);
2727+
right_relids = pull_varnos(root, rightop);
27152728
expr_op = saop->opno;
27162729
expr_coll = saop->inputcollid;
27172730

@@ -2759,7 +2772,8 @@ match_saopclause_to_indexcol(RestrictInfo *rinfo,
27592772
* is handled by expand_indexqual_rowcompare().
27602773
*/
27612774
static IndexClause *
2762-
match_rowcompare_to_indexcol(RestrictInfo *rinfo,
2775+
match_rowcompare_to_indexcol(PlannerInfo *root,
2776+
RestrictInfo *rinfo,
27632777
int indexcol,
27642778
IndexOptInfo *index)
27652779
{
@@ -2804,14 +2818,14 @@ match_rowcompare_to_indexcol(RestrictInfo *rinfo,
28042818
* These syntactic tests are the same as in match_opclause_to_indexcol()
28052819
*/
28062820
if (match_index_to_operand(leftop, indexcol, index) &&
2807-
!bms_is_member(index_relid, pull_varnos(rightop)) &&
2821+
!bms_is_member(index_relid, pull_varnos(root, rightop)) &&
28082822
!contain_volatile_functions(rightop))
28092823
{
28102824
/* OK, indexkey is on left */
28112825
var_on_left = true;
28122826
}
28132827
else if (match_index_to_operand(rightop, indexcol, index) &&
2814-
!bms_is_member(index_relid, pull_varnos(leftop)) &&
2828+
!bms_is_member(index_relid, pull_varnos(root, leftop)) &&
28152829
!contain_volatile_functions(leftop))
28162830
{
28172831
/* indexkey is on right, so commute the operator */
@@ -2830,7 +2844,8 @@ match_rowcompare_to_indexcol(RestrictInfo *rinfo,
28302844
case BTLessEqualStrategyNumber:
28312845
case BTGreaterEqualStrategyNumber:
28322846
case BTGreaterStrategyNumber:
2833-
return expand_indexqual_rowcompare(rinfo,
2847+
return expand_indexqual_rowcompare(root,
2848+
rinfo,
28342849
indexcol,
28352850
index,
28362851
expr_op,
@@ -2864,7 +2879,8 @@ match_rowcompare_to_indexcol(RestrictInfo *rinfo,
28642879
* but we split it out for comprehensibility.
28652880
*/
28662881
static IndexClause *
2867-
expand_indexqual_rowcompare(RestrictInfo *rinfo,
2882+
expand_indexqual_rowcompare(PlannerInfo *root,
2883+
RestrictInfo *rinfo,
28682884
int indexcol,
28692885
IndexOptInfo *index,
28702886
Oid expr_op,
@@ -2942,7 +2958,7 @@ expand_indexqual_rowcompare(RestrictInfo *rinfo,
29422958
if (expr_op == InvalidOid)
29432959
break; /* operator is not usable */
29442960
}
2945-
if (bms_is_member(index->rel->relid, pull_varnos(constop)))
2961+
if (bms_is_member(index->rel->relid, pull_varnos(root, constop)))
29462962
break; /* no good, Var on wrong side */
29472963
if (contain_volatile_functions(constop))
29482964
break; /* no good, volatile comparison value */
@@ -3055,7 +3071,8 @@ expand_indexqual_rowcompare(RestrictInfo *rinfo,
30553071
matching_cols);
30563072
rc->rargs = list_truncate(copyObject(non_var_args),
30573073
matching_cols);
3058-
iclause->indexquals = list_make1(make_simple_restrictinfo((Expr *) rc));
3074+
iclause->indexquals = list_make1(make_simple_restrictinfo(root,
3075+
(Expr *) rc));
30593076
}
30603077
else
30613078
{
@@ -3069,7 +3086,7 @@ expand_indexqual_rowcompare(RestrictInfo *rinfo,
30693086
copyObject(linitial(non_var_args)),
30703087
InvalidOid,
30713088
linitial_oid(clause->inputcollids));
3072-
iclause->indexquals = list_make1(make_simple_restrictinfo(op));
3089+
iclause->indexquals = list_make1(make_simple_restrictinfo(root, op));
30733090
}
30743091
}
30753092

@@ -3686,7 +3703,9 @@ relation_has_unique_index_for(PlannerInfo *root, RelOptInfo *rel,
36863703
* specified index column matches a boolean restriction clause.
36873704
*/
36883705
bool
3689-
indexcol_is_bool_constant_for_query(IndexOptInfo *index, int indexcol)
3706+
indexcol_is_bool_constant_for_query(PlannerInfo *root,
3707+
IndexOptInfo *index,
3708+
int indexcol)
36903709
{
36913710
ListCell *lc;
36923711

@@ -3708,7 +3727,7 @@ indexcol_is_bool_constant_for_query(IndexOptInfo *index, int indexcol)
37083727
continue;
37093728

37103729
/* See if we can match the clause's expression to the index column */
3711-
if (match_boolean_index_clause(rinfo, indexcol, index))
3730+
if (match_boolean_index_clause(root, rinfo, indexcol, index))
37123731
return true;
37133732
}
37143733

@@ -3821,9 +3840,15 @@ match_index_to_operand(Node *operand,
38213840
*/
38223841
bool
38233842
is_pseudo_constant_for_index(Node *expr, IndexOptInfo *index)
3843+
{
3844+
return is_pseudo_constant_for_index_new(NULL, expr, index);
3845+
}
3846+
3847+
bool
3848+
is_pseudo_constant_for_index_new(PlannerInfo *root, Node *expr, IndexOptInfo *index)
38243849
{
38253850
/* pull_varnos is cheaper than volatility check, so do that first */
3826-
if (bms_is_member(index->rel->relid, pull_varnos(expr)))
3851+
if (bms_is_member(index->rel->relid, pull_varnos(root, expr)))
38273852
return false; /* no good, contains Var of table */
38283853
if (contain_volatile_functions(expr))
38293854
return false; /* no good, volatile comparison value */

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -542,7 +542,7 @@ build_index_pathkeys(PlannerInfo *root,
542542
* should stop considering index columns; any lower-order sort
543543
* keys won't be useful either.
544544
*/
545-
if (!indexcol_is_bool_constant_for_query(index, i))
545+
if (!indexcol_is_bool_constant_for_query(root, index, i))
546546
break;
547547
}
548548

0 commit comments

Comments
 (0)