8000 Fix pull_varnos' miscomputation of relids set for a PlaceHolderVar. · postgrespro/postgres@55dc86e · GitHub
[go: up one dir, main page]
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 8000 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

File tree

25 files changed

+261
-131
lines changed
  • var.c
  • utils/adt
  • include/optimizer
  • test/regress

    • 25 files changed

    +261
    -131
    lines changed

    ‎contrib/postgres_fdw/postgres_fdw.c

    Lines changed: 2 additions & 1 deletion
    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 9E88

    Lines changed: 6 additions & 6 deletions
    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

    Lines changed: 1 addition & 1 deletion
    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

    Lines changed: 11 additions & 6 deletions
    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

    Lines changed: 37 additions & 24 deletions
    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+
    i C2EE f (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

    Lines changed: 1 addition & 1 deletion
    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)
    0