From 043b7525590278fabacd3c10ef869b1ef2b7358f Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Fri, 12 Jul 2019 22:26:20 +0200 Subject: [PATCH 1/2] Determine operator semantics using btree opclasses --- src/backend/statistics/dependencies.c | 15 +++--- src/backend/statistics/extended_stats.c | 69 +++++++++++++++++++------ src/backend/statistics/mcv.c | 60 ++++++++++++--------- src/include/statistics/statistics.h | 1 + 4 files changed, 99 insertions(+), 46 deletions(-) diff --git a/src/backend/statistics/dependencies.c b/src/backend/statistics/dependencies.c index 66c38ce2bc..2723ca33df 100644 --- a/src/backend/statistics/dependencies.c +++ b/src/backend/statistics/dependencies.c @@ -14,6 +14,7 @@ #include "postgres.h" #include "access/htup_details.h" +#include "access/stratnum.h" #include "access/sysattr.h" #include "catalog/pg_operator.h" #include "catalog/pg_statistic_ext.h" @@ -788,15 +789,13 @@ dependency_is_compatible_clause(Node *clause, Index relid, AttrNumber *attnum) * If it's not an "=" operator, just ignore the clause, as it's not * compatible with functional dependencies. * - * This uses the function for estimating selectivity, not the operator - * directly (a bit awkward, but well ...). - * - * XXX this is pretty dubious; probably it'd be better to check btree - * or hash opclass membership, so as not to be fooled by custom - * selectivity functions, and to be more consistent with decisions - * elsewhere in the planner. + * To decide this, we need to decide the semantics of the operator. + * We do that by matching it to btree opclasses, and checking the + * strategy numbers. When there's no btree opclass, or when there + * are multiple strategy numbers, we consider the semantics to be + * unclear (i.e. not an equality operator). */ - if (get_oprrest(expr->opno) != F_EQSEL) + if (determine_operator_strategy(expr->opno) != BTEqualStrategyNumber) return false; /* OK to proceed with checking "var" */ diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c index c56ed48270..2ed28c054f 100644 --- a/src/backend/statistics/extended_stats.c +++ b/src/backend/statistics/extended_stats.c @@ -20,6 +20,7 @@ #include "access/htup_details.h" #include "access/table.h" #include "access/tuptoaster.h" +#include "access/stratnum.h" #include "catalog/indexing.h" #include "catalog/pg_collation.h" #include "catalog/pg_statistic_ext.h" @@ -819,22 +820,14 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause, * * This uses the function for estimating selectivity, not the operator * directly (a bit awkward, but well ...). + * + * XXX We can't check this against strategies defined in stratnum.h, + * because get_op_btree_interpretation() introduces a new strategy + * number for operator '<>' and we'd miss that. So we simply check + * if the result is (-1) which means strategy is unknown. */ - switch (get_oprrest(expr->opno)) - { - case F_EQSEL: - case F_NEQSEL: - case F_SCALARLTSEL: - case F_SCALARLESEL: - case F_SCALARGTSEL: - case F_SCALARGESEL: - /* supported, will continue with inspection of the Var */ - break; - - default: - /* other estimators are considered unknown/unsupported */ - return false; - } + if (determine_operator_strategy(expr->opno) == -1) + return false; /* * If there are any securityQuals on the RTE from security barrier @@ -1196,3 +1189,49 @@ statext_clauselist_selectivity(PlannerInfo *root, List *clauses, int varRelid, return sel; } + +/* + * determine_operator_strategy + * Determine operator semantics by matching it to btree opclasses. + * + * We look at all the btree opclasses referencing the supplied operator, and + * check if the strategy number is the same in all of them. If yes, we consider + * that to be the semantics of the operator (equality, less-than, ...) and + * return that strategy number. + * + * If there are no matching btree opclasses, or when the operator has multiple + * strategy numbers in various opclasses, we consider the operator semantics to + * be unclear/undefined and return -1. + * + * Note: This only looks at btree opclasses, so for data types with only a hash + * opclass, this always returns (-1). That ultimately means we won't be able + * to use extended statistics for them. + * + * Note: The result strategy number may not be defined in stratnum.h, so you + * need to be careful when checking the result. This is because this calls + * get_op_btree_interpretation which may introduce new stragies that are not + * actually part of btree definition (e.g. '<>') + */ +int +determine_operator_strategy(Oid opno) +{ + ListCell *lc; + int strat; + List *opinfos; + Bitmapset *strats = NULL; + + opinfos = get_op_btree_interpretation(opno); + + foreach(lc, opinfos) + { + OpBtreeInterpretation *opinfo = lfirst(lc); + + strats = bms_add_member(strats, opinfo->strategy); + } + + /* multiple (or multiple) strategies means unclear semantics */ + if (!bms_get_singleton_member(strats, &strat)) + return -1; + + return strat; +} diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c index 913a72ff67..1e919c82f9 100644 --- a/src/backend/statistics/mcv.c +++ b/src/backend/statistics/mcv.c @@ -1565,9 +1565,6 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses, bool ok; FmgrInfo opproc; - /* get procedure computing operator selectivity */ - RegProcedure oprrest = get_oprrest(expr->opno); - fmgr_info(get_opcode(expr->opno), &opproc); ok = (NumRelids(clause) == 1) && @@ -1583,6 +1580,7 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses, Const *cst; bool isgt; int idx; + int strategy; /* extract the var and const from the expression */ var = (varonleft) ? linitial(expr->args) : lsecond(expr->args); @@ -1600,6 +1598,16 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses, typecache = lookup_type_cache(var->vartype, TYPECACHE_GT_OPR); fmgr_info(get_opcode(typecache->gt_opr), >proc); + /* determine operator semantics from btree opclasses */ + strategy = determine_operator_strategy(expr->opno); + + /* + * At this point all the expressions must have a meaningful + * btree opclass interpretation (thanks to passing through + * statext_is_compatible_clause, which enforces that). + */ + Assert(strategy > 0); + /* * Walk through the MCV items and evaluate the current clause. * We can skip items that were already ruled out, and @@ -1625,27 +1633,12 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses, else if (is_or && (matches[i] == true)) continue; - switch (oprrest) + switch (strategy) { - 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). - */ - mismatch = !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 */ + case BTLessStrategyNumber: /* column < constant */ + case BTLessEqualStrategyNumber: /* column <= constant */ + case BTGreaterEqualStrategyNumber: /* column > constant */ + case BTGreaterStrategyNumber: /* column >= constant */ /* * First check whether the constant is below the @@ -1664,6 +1657,27 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses, cst->constvalue)); break; + + default: + + /* + * This does handle equality and inequality, but we handle + * that as a default case because inequality operator '<>' + * does not actually have a btree opclass strategy, it's + * added by get_op_btree_interpretation. + */ + + /* + * We don't care about isgt in equality, because + * it does not matter whether it's (var op const) + * or (const op var). + */ + mismatch = !DatumGetBool(FunctionCall2Coll(&opproc, + DEFAULT_COLLATION_OID, + cst->constvalue, + item->values[idx])); + + break; } /* diff --git a/src/include/statistics/statistics.h b/src/include/statistics/statistics.h index cb7bc630e9..82e2d5c619 100644 --- a/src/include/statistics/statistics.h +++ b/src/include/statistics/statistics.h @@ -118,5 +118,6 @@ extern Selectivity statext_clauselist_selectivity(PlannerInfo *root, extern bool has_stats_of_kind(List *stats, char requiredkind); extern StatisticExtInfo *choose_best_statistics(List *stats, Bitmapset *attnums, char requiredkind); +extern int determine_operator_strategy(Oid opno); #endif /* STATISTICS_H */ -- 2.20.1