agora inbox for [email protected]
help / color / mirror / Atom feedFrom: Tomas Vondra <[email protected]>
Subject: [PATCH 1/2] Determine operator semantics using btree opclasses
Date: Fri, 12 Jul 2019 22:26:20 +0200
---
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
--omnnplrhcrdpixl2
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
filename="0002-Fix-processing-of-OpExpr-in-extended-statistics.patch"
view thread (15+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected]
Subject: Re: [PATCH 1/2] Determine operator semantics using btree opclasses
In-Reply-To: <no-message-id-1882042@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox