From 043b7525590278fabacd3c10ef869b1ef2b7358f Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@2ndquadrant.com>
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), &gtproc);
 
+				/* 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

