From ea6acd6285bf697a3a838c6df163aa4a3466a769 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Sat, 13 Jul 2019 00:12:16 +0200 Subject: [PATCH 2/5] Fix handling of opclauses in extended statistics We expect opclauses to have exactly one Var and one Const, but the code was checking the Const by calling is_pseudo_constant_clause() which is incorrect - we need a proper constant. Fixed by using plain IsA(x,Const) to check type of the node. We need to do these checks in two places, so move it into a separate function that can be called in both places. Reported by Andreas Seltenreich, based on crash reported by sqlsmith. Discussion: https://postgr.es/m/8736jdhbhc.fsf%40ansel.ydns.eu --- src/backend/statistics/extended_stats.c | 74 ++++++++++++++++--- src/backend/statistics/mcv.c | 27 ++----- .../statistics/extended_stats_internal.h | 2 + 3 files changed, 71 insertions(+), 32 deletions(-) diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c index c56ed48270..08032f1b28 100644 --- a/src/backend/statistics/extended_stats.c +++ b/src/backend/statistics/extended_stats.c @@ -796,21 +796,13 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause, RangeTblEntry *rte = root->simple_rte_array[relid]; OpExpr *expr = (OpExpr *) clause; Var *var; - bool varonleft = true; - bool ok; /* Only expressions with two arguments are considered compatible. */ if (list_length(expr->args) != 2) return false; - /* see if it actually has the right shape (one Var, one Const) */ - ok = (NumRelids((Node *) expr) == 1) && - (is_pseudo_constant_clause(lsecond(expr->args)) || - (varonleft = false, - is_pseudo_constant_clause(linitial(expr->args)))); - - /* unsupported structure (two variables or so) */ - if (!ok) + /* Check if the expression the right shape (one Var, one Const) */ + if (!examine_opclause_expression(expr, &var, NULL, NULL)) return false; /* @@ -850,8 +842,6 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause, !get_func_leakproof(get_opcode(expr->opno))) return false; - var = (varonleft) ? linitial(expr->args) : lsecond(expr->args); - return statext_is_compatible_clause_internal(root, (Node *) var, relid, attnums); } @@ -1196,3 +1186,63 @@ statext_clauselist_selectivity(PlannerInfo *root, List *clauses, int varRelid, return sel; } + +/* + * examine_operator_expression + * Split expression into Var and Const parts. + * + * Attempts to match the arguments to either (Var op Const) or (Const op Var), + * possibly with a RelabelType on top of the Var node. When successful, returns + * true, otherwise returns false. + * + * Optionally returns pointers to the extracted elements, when passed non-null + * pointers (varp, cstp and isgtp). + */ +bool +examine_opclause_expression(OpExpr *expr, Var **varp, Const **cstp, bool *isgtp) +{ + Var *var; + Const *cst; + bool isgt; + Node *leftop, + *rightop; + + /* enforced by statext_is_compatible_clause_internal */ + Assert(list_length(expr->args) == 2); + + leftop = linitial(expr->args); + rightop = lsecond(expr->args); + + /* strip RelabelType from either side of the expression */ + if (IsA(leftop, RelabelType)) + leftop = (Node *) ((RelabelType *) leftop)->arg; + + if (IsA(rightop, RelabelType)) + rightop = (Node *) ((RelabelType *) rightop)->arg; + + if (IsA(leftop, Var) && IsA(rightop, Const)) + { + var = (Var *) leftop; + cst = (Const *) rightop; + isgt = false; + } + else if (IsA(leftop, Const) && IsA(rightop, Var)) + { + var = (Var *) rightop; + cst = (Const *) leftop; + isgt = true; + } + else + return false; + + if (varp) + *varp = var; + + if (cstp) + *cstp = cst; + + if (isgtp) + *isgtp = isgt; + + return true; +} diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c index 2e375edcb4..865981bbb4 100644 --- a/src/backend/statistics/mcv.c +++ b/src/backend/statistics/mcv.c @@ -1561,36 +1561,23 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses, if (is_opclause(clause)) { OpExpr *expr = (OpExpr *) clause; - bool varonleft = true; - bool ok; FmgrInfo opproc; /* get procedure computing operator selectivity */ RegProcedure oprrest = get_oprrest(expr->opno); - fmgr_info(get_opcode(expr->opno), &opproc); + /* valid only after examine_opclause_expression returns true */ + Var *var; + Const *cst; + bool isgt; - ok = (NumRelids(clause) == 1) && - (is_pseudo_constant_clause(lsecond(expr->args)) || - (varonleft = false, - is_pseudo_constant_clause(linitial(expr->args)))); + fmgr_info(get_opcode(expr->opno), &opproc); - if (ok) + /* extract the var and const from the expression */ + if (examine_opclause_expression(expr, &var, &cst, &isgt)) { - Var *var; - Const *cst; - bool isgt; int idx; - /* extract the var and const from the expression */ - var = (varonleft) ? linitial(expr->args) : lsecond(expr->args); - cst = (varonleft) ? lsecond(expr->args) : linitial(expr->args); - isgt = (!varonleft); - - /* strip binary-compatible relabeling */ - if (IsA(var, RelabelType)) - var = (Var *) ((RelabelType *) var)->arg; - /* match the attribute to a dimension of the statistic */ idx = bms_member_index(keys, var->varattno); diff --git a/src/include/statistics/extended_stats_internal.h b/src/include/statistics/extended_stats_internal.h index 8fc541993f..c7f01d4edc 100644 --- a/src/include/statistics/extended_stats_internal.h +++ b/src/include/statistics/extended_stats_internal.h @@ -97,6 +97,8 @@ extern SortItem *build_sorted_items(int numrows, int *nitems, HeapTuple *rows, TupleDesc tdesc, MultiSortSupport mss, int numattrs, AttrNumber *attnums); +extern bool examine_opclause_expression(OpExpr *expr, Var **varp, + Const **cstp, bool *isgtp); extern Selectivity mcv_clauselist_selectivity(PlannerInfo *root, StatisticExtInfo *stat, -- 2.20.1