From 14dc49ad7eefd6e67b751e74ce7253cdc85ca5ad Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Fri, 19 Jul 2019 16:28:28 +0200 Subject: [PATCH 1/2] Rework examine_opclause_expression to use varonleft The examine_opclause_expression function needs to return information on which side of the operator we found the Var, but the variable was called "isgt" which is rather misleading (it assumes the operator is either less-than or greater-than, but it may be equality or something else). Other places in the planner use a variable called "varonleft" for this purpose, so just adopt the same convention here. The code also assumed we don't care about this flag for equality, as (Var = Const) and (Const = Var) should be the same thing. But that does not work for cross-type operators, in which case we need to pass the parameters to the procedure in the right order. So just use the same code for all types of expressions. This means we don't need to care about the selectivity estimation function anymore, at least not in this code. We should only get the supported cases here (thanks to statext_is_compatible_clause). Discussion: https://postgr.es/m/8736jdhbhc.fsf%40ansel.ydns.eu Backpatch-to: 12 --- src/backend/statistics/extended_stats.c | 16 ++--- src/backend/statistics/mcv.c | 62 +++++-------------- .../statistics/extended_stats_internal.h | 2 +- 3 files changed, 26 insertions(+), 54 deletions(-) diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c index d2346cbe02..23c74f7d90 100644 --- a/src/backend/statistics/extended_stats.c +++ b/src/backend/statistics/extended_stats.c @@ -1196,15 +1196,15 @@ statext_clauselist_selectivity(PlannerInfo *root, List *clauses, int varRelid, * returns true, otherwise returns false. * * Optionally returns pointers to the extracted Var/Const nodes, when passed - * non-null pointers (varp, cstp and isgtp). The isgt flag specifies whether - * the Var node is on the left (false) or right (true) side of the operator. + * non-null pointers (varp, cstp and varonleftp). The varonleftp flag specifies + * on which side of the operator we found the Var node. */ bool -examine_opclause_expression(OpExpr *expr, Var **varp, Const **cstp, bool *isgtp) +examine_opclause_expression(OpExpr *expr, Var **varp, Const **cstp, bool *varonleftp) { Var *var; Const *cst; - bool isgt; + bool varonleft; Node *leftop, *rightop; @@ -1225,13 +1225,13 @@ examine_opclause_expression(OpExpr *expr, Var **varp, Const **cstp, bool *isgtp) { var = (Var *) leftop; cst = (Const *) rightop; - isgt = false; + varonleft = true; } else if (IsA(leftop, Const) && IsA(rightop, Var)) { var = (Var *) rightop; cst = (Const *) leftop; - isgt = true; + varonleft = false; } else return false; @@ -1243,8 +1243,8 @@ examine_opclause_expression(OpExpr *expr, Var **varp, Const **cstp, bool *isgtp) if (cstp) *cstp = cst; - if (isgtp) - *isgtp = isgt; + if (varonleftp) + *varonleftp = varonleft; return true; } diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c index e5a4e86c5d..2b685ec67a 100644 --- a/src/backend/statistics/mcv.c +++ b/src/backend/statistics/mcv.c @@ -1581,18 +1581,15 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses, OpExpr *expr = (OpExpr *) clause; FmgrInfo opproc; - /* get procedure computing operator selectivity */ - RegProcedure oprrest = get_oprrest(expr->opno); - /* valid only after examine_opclause_expression returns true */ Var *var; Const *cst; - bool isgt; + bool varonleft; fmgr_info(get_opcode(expr->opno), &opproc); /* extract the var and const from the expression */ - if (examine_opclause_expression(expr, &var, &cst, &isgt)) + if (examine_opclause_expression(expr, &var, &cst, &varonleft)) { int idx; @@ -1629,46 +1626,21 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses, if (RESULT_IS_FINAL(matches[i], is_or)) continue; - switch (oprrest) - { - case F_EQSEL: - case F_NEQSEL: - - /* - * We don't care about isgt in equality, because - * it does not matter whether it's (var op const) - * or (const op var). - */ - match = DatumGetBool(FunctionCall2Coll(&opproc, - DEFAULT_COLLATION_OID, - cst->constvalue, - item->values[idx])); - - break; - - case F_SCALARLTSEL: /* column < constant */ - case F_SCALARLESEL: /* column <= constant */ - case F_SCALARGTSEL: /* column > constant */ - case F_SCALARGESEL: /* column >= constant */ - - /* - * First check whether the constant is below the - * lower boundary (in that case we can skip the - * bucket, because there's no overlap). - */ - if (isgt) - match = DatumGetBool(FunctionCall2Coll(&opproc, - DEFAULT_COLLATION_OID, - cst->constvalue, - item->values[idx])); - else - match = DatumGetBool(FunctionCall2Coll(&opproc, - DEFAULT_COLLATION_OID, - item->values[idx], - cst->constvalue)); - - break; - } + /* + * First check whether the constant is below the lower + * boundary (in that case we can skip the bucket, because + * there's no overlap). + */ + if (varonleft) + match = DatumGetBool(FunctionCall2Coll(&opproc, + DEFAULT_COLLATION_OID, + item->values[idx], + cst->constvalue)); + else + match = DatumGetBool(FunctionCall2Coll(&opproc, + DEFAULT_COLLATION_OID, + cst->constvalue, + item->values[idx])); /* update the match bitmap with the result */ matches[i] = RESULT_MERGE(matches[i], is_or, match); diff --git a/src/include/statistics/extended_stats_internal.h b/src/include/statistics/extended_stats_internal.h index c7f01d4edc..8433c34f6d 100644 --- a/src/include/statistics/extended_stats_internal.h +++ b/src/include/statistics/extended_stats_internal.h @@ -98,7 +98,7 @@ extern SortItem *build_sorted_items(int numrows, int *nitems, HeapTuple *rows, int numattrs, AttrNumber *attnums); extern bool examine_opclause_expression(OpExpr *expr, Var **varp, - Const **cstp, bool *isgtp); + Const **cstp, bool *varonleftp); extern Selectivity mcv_clauselist_selectivity(PlannerInfo *root, StatisticExtInfo *stat, -- 2.21.0