Fix up btree's initial scankey processing to be able to detect redundant

or contradictory keys even in cross-data-type scenarios.  This is another
benefit of the opfamily rewrite: we can find the needed comparison
operators now.

src/backend/access/nbtree/README

@@ -1,4 +1,4 @@
-$PostgreSQL: pgsql/src/backend/access/nbtree/README,v 1.14 2006/11/01 19:43:17 tgl Exp $
+$PostgreSQL: pgsql/src/backend/access/nbtree/README,v 1.15 2006/12/28 23:16:39 tgl Exp $
 
 This directory contains a correct implementation of Lehman and Yao's
 high-concurrency B-tree management algorithm (P. Lehman and S. Yao,
@@ -480,7 +480,8 @@ than key.
 Notes to operator class implementors
 ------------------------------------
 
-With this implementation, we require each supported datatype to supply
+With this implementation, we require each supported combination of
-us with a comparison procedure via pg_amproc.  This procedure must take
+datatypes to supply us with a comparison procedure via pg_amproc.
-two nonnull values A and B and return an int32 < 0, 0, or > 0 if A < B,
+This procedure must take two nonnull values A and B and return an int32 < 0,
-A = B, or A > B, respectively.  See nbtcompare.c for examples.
+0, or > 0 if A < B, A = B, or A > B, respectively.  See nbtcompare.c for
+examples.

src/backend/access/nbtree/nbtsearch.c

@@ -8,7 +8,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/nbtree/nbtsearch.c,v 1.108 2006/12/23 00:43:09 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/nbtree/nbtsearch.c,v 1.109 2006/12/28 23:16:39 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -476,16 +476,16 @@ _bt_first(IndexScanDesc scan, ScanDirection dir)
 	 * attributes to its right, because it would break our simplistic notion
 	 * of what initial positioning strategy to use.
 	 *
-	 * When the scan keys include non-default operators, _bt_preprocess_keys
+	 * When the scan keys include cross-type operators, _bt_preprocess_keys
 	 * may not be able to eliminate redundant keys; in such cases we will
 	 * arbitrarily pick a usable one for each attribute.  This is correct
 	 * but possibly not optimal behavior.  (For example, with keys like
 	 * "x >= 4 AND x >= 5" we would elect to scan starting at x=4 when
-	 * x=5 would be more efficient.)  Since the situation only arises in
+	 * x=5 would be more efficient.)  Since the situation only arises given
-	 * hokily-worded queries, live with it.
+	 * a poorly-worded query plus an incomplete opfamily, live with it.
 	 *
 	 * When both equality and inequality keys appear for a single attribute
-	 * (again, only possible when non-default operators appear), we *must*
+	 * (again, only possible when cross-type operators appear), we *must*
 	 * select one of the equality keys for the starting point, because
 	 * _bt_checkkeys() will stop the scan as soon as an equality qual fails.
 	 * For example, if we have keys like "x >= 4 AND x = 10" and we elect to
@@ -658,11 +658,15 @@ _bt_first(IndexScanDesc scan, ScanDirection dir)
 			 * to an insertion scan key by replacing the sk_func with the
 			 * appropriate btree comparison function.
 			 *
-			 * If scankey operator is of the default type for the index, we
+			 * If scankey operator is not a cross-type comparison, we can use
-			 * can use the cached comparison function; otherwise gotta look it
+			 * the cached comparison function; otherwise gotta look it up in
-			 * up in the catalogs.  Also, we support the convention that
+			 * the catalogs.  (That can't lead to infinite recursion, since no
-			 * sk_subtype == 0 means the default type; this is a hack to
+			 * indexscan initiated by syscache lookup will use cross-data-type
-			 * simplify life for ScanKeyInit().
+			 * operators.)
+			 *
+			 * We support the convention that sk_subtype == InvalidOid means
+			 * the opclass input type; this is a hack to simplify life for
+			 * ScanKeyInit().
 			 */
 			if (cur->sk_subtype == rel->rd_opcintype[i] ||
 				cur->sk_subtype == InvalidOid)

src/backend/access/nbtree/nbtutils.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/nbtree/nbtutils.c,v 1.79 2006/10/04 00:29:49 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/nbtree/nbtutils.c,v 1.80 2006/12/28 23:16:39 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -24,8 +24,12 @@
 #include "miscadmin.h"
 #include "storage/lwlock.h"
 #include "storage/shmem.h"
+#include "utils/lsyscache.h"
 
 
+static bool _bt_compare_scankey_args(IndexScanDesc scan, ScanKey op,
+						 ScanKey leftarg, ScanKey rightarg,
+						 bool *result);
 static void _bt_mark_scankey_required(ScanKey skey);
 static bool _bt_check_rowcompare(ScanKey skey,
 					 IndexTuple tuple, TupleDesc tupdesc,
@@ -145,7 +149,7 @@ _bt_freestack(BTStack stack)
 }
 
 
-/*----------
+/*
  *	_bt_preprocess_keys() -- Preprocess scan keys
  *
  * The caller-supplied search-type keys (in scan->keyData[]) are copied to
@@ -155,9 +159,9 @@ _bt_freestack(BTStack stack)
  *
  * The primary purpose of this routine is to discover how many scan keys
  * must be satisfied to continue the scan.	It also attempts to eliminate
- * redundant keys and detect contradictory keys.  At present, redundant and
+ * redundant keys and detect contradictory keys.  (If the index opfamily
- * contradictory keys can only be detected for same-data-type comparisons,
+ * provides incomplete sets of cross-type operators, we may fail to detect
- * but that's the usual case so it seems worth doing.
+ * redundant or contradictory keys, but we can survive that.)
  *
  * The output keys must be sorted by index attribute.  Presently we expect
  * (but verify) that the input keys are already so sorted --- this is done
@@ -184,25 +188,23 @@ _bt_freestack(BTStack stack)
  * If possible, redundant keys are eliminated: we keep only the tightest
  * >/>= bound and the tightest </<= bound, and if there's an = key then
  * that's the only one returned.  (So, we return either a single = key,
- * or one or two boundary-condition keys for each attr.)  However, we can
+ * or one or two boundary-condition keys for each attr.)  However, if we
- * only detect redundant keys when the right-hand datatypes are all equal
+ * cannot compare two keys for lack of a suitable cross-type operator,
- * to the index datatype, because we do not know suitable operators for
+ * we cannot eliminate either.  If there are two such keys of the same
- * comparing right-hand values of two different datatypes.	(In theory
+ * operator strategy, the second one is just pushed into the output array
- * we could handle comparison of a RHS of the index datatype with a RHS of
+ * without further processing here.  We may also emit both >/>= or both
- * another type, but that seems too much pain for too little gain.)  So,
+ * </<= keys if we can't compare them.  The logic about required keys still
- * keys whose operator has a nondefault subtype (ie, its RHS is not of the
+ * works if we don't eliminate redundant keys.
- * index datatype) are ignored here, except for noting whether they include
- * an "=" condition or not.  The logic about required keys still works if
- * we don't eliminate redundant keys.
  *
  * As a byproduct of this work, we can detect contradictory quals such
- * as "x = 1 AND x > 2".  If we see that, we return so->quals_ok = FALSE,
+ * as "x = 1 AND x > 2".  If we see that, we return so->qual_ok = FALSE,
  * indicating the scan need not be run at all since no tuples can match.
- * Again though, only keys with RHS datatype equal to the index datatype
+ * (In this case we do not bother completing the output key array!)
- * can be checked for contradictions.
+ * Again, missing cross-type operators might cause us to fail to prove the
+ * quals contradictory when they really are, but the scan will work correctly.
  *
- * Row comparison keys are treated the same as comparisons to nondefault
+ * Row comparison keys are currently also treated without any smarts:
- * datatypes: we just transfer them into the preprocessed array without any
+ * we just transfer them into the preprocessed array without any
  * editorialization.  We can treat them the same as an ordinary inequality
  * comparison on the row's first index column, for the purposes of the logic
  * about required keys.
@@ -211,7 +213,6 @@ _bt_freestack(BTStack stack)
  * storage is that we are modifying the array based on comparisons of the
  * key argument values, which could change on a rescan.  Therefore we can't
  * overwrite the caller's data structure.
- *----------
  */
 void
 _bt_preprocess_keys(IndexScanDesc scan)
@@ -224,8 +225,7 @@ _bt_preprocess_keys(IndexScanDesc scan)
 	ScanKey		outkeys;
 	ScanKey		cur;
 	ScanKey		xform[BTMaxStrategyNumber];
-	bool		hasOtherTypeEqual;
+	bool		test_result;
-	Datum		test;
 	int			i,
 				j;
 	AttrNumber	attno;
@@ -271,15 +271,11 @@ _bt_preprocess_keys(IndexScanDesc scan)
 	/*
 	 * Initialize for processing of keys for attr 1.
 	 *
-	 * xform[i] points to the currently best scan key of strategy type i+1, if
+	 * xform[i] points to the currently best scan key of strategy type i+1;
-	 * any is found with a default operator subtype; it is NULL if we haven't
+	 * it is NULL if we haven't yet found such a key for this attr.
-	 * yet found such a key for this attr.	Scan keys of nondefault subtypes
-	 * are transferred to the output with no processing except for noting if
-	 * they are of "=" type.
 	 */
 	attno = 1;
 	memset(xform, 0, sizeof(xform));
-	hasOtherTypeEqual = false;
 
 	/*
 	 * Loop iterates from 0 to numberOfKeys inclusive; we use the last pass to
@@ -317,9 +313,9 @@ _bt_preprocess_keys(IndexScanDesc scan)
 				elog(ERROR, "btree index keys must be ordered by attribute");
 
 			/*
-			 * If = has been specified, no other key will be used. In case of
+			 * If = has been specified, all other keys can be eliminated as
-			 * key > 2 && key == 1 and so on we have to set qual_ok to false
+			 * redundant.  In case of key > 2 && key == 1 we can set qual_ok
-			 * before discarding the other keys.
+			 * to false and abandon further processing.
 			 */
 			if (xform[BTEqualStrategyNumber - 1])
 			{
@@ -331,60 +327,57 @@ _bt_preprocess_keys(IndexScanDesc scan)
 
 					if (!chk || j == (BTEqualStrategyNumber - 1))
 						continue;
-					test = FunctionCall2(&chk->sk_func,
+					if (_bt_compare_scankey_args(scan, chk, eq, chk,
-										 eq->sk_argument,
+												 &test_result))
-										 chk->sk_argument);
-					if (!DatumGetBool(test))
 					{
+						if (!test_result)
+						{
+							/* keys proven mutually contradictory */
 							so->qual_ok = false;
-						break;
+							return;
 						}
+						/* else discard the redundant non-equality key */
+						xform[j] = NULL;
+					}
+					/* else, cannot determine redundancy, keep both keys */
 				}
-				xform[BTLessStrategyNumber - 1] = NULL;
-				xform[BTLessEqualStrategyNumber - 1] = NULL;
-				xform[BTGreaterEqualStrategyNumber - 1] = NULL;
-				xform[BTGreaterStrategyNumber - 1] = NULL;
 				/* track number of attrs for which we have "=" keys */
 				numberOfEqualCols++;
 			}
-			else
-			{
-				/* track number of attrs for which we have "=" keys */
-				if (hasOtherTypeEqual)
-					numberOfEqualCols++;
-			}
 
-			/* keep only one of <, <= */
+			/* try to keep only one of <, <= */
 			if (xform[BTLessStrategyNumber - 1]
 				&& xform[BTLessEqualStrategyNumber - 1])
 			{
 				ScanKey		lt = xform[BTLessStrategyNumber - 1];
 				ScanKey		le = xform[BTLessEqualStrategyNumber - 1];
 
-				test = FunctionCall2(&le->sk_func,
+				if (_bt_compare_scankey_args(scan, le, lt, le,
-									 lt->sk_argument,
+											 &test_result))
-									 le->sk_argument);
+				{
-				if (DatumGetBool(test))
+					if (test_result)
 						xform[BTLessEqualStrategyNumber - 1] = NULL;
 					else
 						xform[BTLessStrategyNumber - 1] = NULL;
 				}
+			}
 
-			/* keep only one of >, >= */
+			/* try to keep only one of >, >= */
 			if (xform[BTGreaterStrategyNumber - 1]
 				&& xform[BTGreaterEqualStrategyNumber - 1])
 			{
 				ScanKey		gt = xform[BTGreaterStrategyNumber - 1];
 				ScanKey		ge = xform[BTGreaterEqualStrategyNumber - 1];
 
-				test = FunctionCall2(&ge->sk_func,
+				if (_bt_compare_scankey_args(scan, ge, gt, ge,
-									 gt->sk_argument,
+											 &test_result))
-									 ge->sk_argument);
+				{
-				if (DatumGetBool(test))
+					if (test_result)
 						xform[BTGreaterEqualStrategyNumber - 1] = NULL;
 					else
 						xform[BTGreaterStrategyNumber - 1] = NULL;
 				}
+			}
 
 			/*
 			 * Emit the cleaned-up keys into the outkeys[] array, and then
@@ -412,33 +405,40 @@ _bt_preprocess_keys(IndexScanDesc scan)
 			/* Re-initialize for new attno */
 			attno = cur->sk_attno;
 			memset(xform, 0, sizeof(xform));
-			hasOtherTypeEqual = false;
 		}
 
 		/* check strategy this key's operator corresponds to */
 		j = cur->sk_strategy - 1;
 
-		/* if row comparison or wrong RHS data type, punt */
+		/* if row comparison, push it directly to the output array */
-		if ((cur->sk_flags & SK_ROW_HEADER) || cur->sk_subtype != InvalidOid)
+		if (cur->sk_flags & SK_ROW_HEADER)
 		{
 			ScanKey		outkey = &outkeys[new_numberOfKeys++];
 
 			memcpy(outkey, cur, sizeof(ScanKeyData));
 			if (numberOfEqualCols == attno - 1)
 				_bt_mark_scankey_required(outkey);
-			if (j == (BTEqualStrategyNumber - 1))
+			/*
-				hasOtherTypeEqual = true;
+			 * We don't support RowCompare using equality; such a qual would
+			 * mess up the numberOfEqualCols tracking.
+			 */
+			Assert(j != (BTEqualStrategyNumber - 1));
 			continue;
 		}
 
 		/* have we seen one of these before? */
-		if (xform[j])
+		if (xform[j] == NULL)
 		{
-			/* yup, keep the more restrictive key */
+			/* nope, so remember this scankey */
-			test = FunctionCall2(&cur->sk_func,
+			xform[j] = cur;
-								 cur->sk_argument,
+		}
-								 xform[j]->sk_argument);
+		else
-			if (DatumGetBool(test))
+		{
+			/* yup, keep only the more restrictive key */
+			if (_bt_compare_scankey_args(scan, cur, cur, xform[j],
+										 &test_result))
+			{
+				if (test_result)
 					xform[j] = cur;
 				else if (j == (BTEqualStrategyNumber - 1))
 				{
@@ -446,17 +446,121 @@ _bt_preprocess_keys(IndexScanDesc scan)
 					so->qual_ok = false;
 					return;
 				}
+				/* else old key is more restrictive, keep it */
 			}
 			else
 			{
-			/* nope, so remember this scankey */
+				/*
-			xform[j] = cur;
+				 * We can't determine which key is more restrictive.  Keep
+				 * the previous one in xform[j] and push this one directly
+				 * to the output array.
+				 */
+				ScanKey		outkey = &outkeys[new_numberOfKeys++];
+
+				memcpy(outkey, cur, sizeof(ScanKeyData));
+				if (numberOfEqualCols == attno - 1)
+					_bt_mark_scankey_required(outkey);
+			}
 		}
 	}
 
 	so->numberOfKeys = new_numberOfKeys;
 }
 
+/*
+ * Compare two scankey values using a specified operator.  Both values
+ * must be already known non-NULL.
+ *
+ * The test we want to perform is logically "leftarg op rightarg", where
+ * leftarg and rightarg are the sk_argument values in those ScanKeys, and
+ * the comparison operator is the one in the op ScanKey.  However, in
+ * cross-data-type situations we may need to look up the correct operator in
+ * the index's opfamily: it is the one having amopstrategy = op->sk_strategy
+ * and amoplefttype/amoprighttype equal to the two argument datatypes.
+ *
+ * If the opfamily doesn't supply a complete set of cross-type operators we
+ * may not be able to make the comparison.  If we can make the comparison
+ * we store the operator result in *result and return TRUE.  We return FALSE
+ * if the comparison could not be made.
+ *
+ * Note: op always points at the same ScanKey as either leftarg or rightarg.
+ * Since we don't scribble on the scankeys, this aliasing should cause no
+ * trouble.
+ */
+static bool
+_bt_compare_scankey_args(IndexScanDesc scan, ScanKey op,
+						 ScanKey leftarg, ScanKey rightarg,
+						 bool *result)
+{
+	Relation	rel = scan->indexRelation;
+	Oid			lefttype,
+				righttype,
+				optype,
+				opcintype,
+				cmp_op;
+
+	/*
+	 * The opfamily we need to worry about is identified by the index column.
+	 */
+	Assert(leftarg->sk_attno == rightarg->sk_attno);
+
+	opcintype = rel->rd_opcintype[leftarg->sk_attno - 1];
+
+	/*
+	 * Determine the actual datatypes of the ScanKey arguments.  We have to
+	 * support the convention that sk_subtype == InvalidOid means the opclass
+	 * input type; this is a hack to simplify life for ScanKeyInit().
+	 */
+	lefttype = leftarg->sk_subtype;
+	if (lefttype == InvalidOid)
+		lefttype = opcintype;
+	righttype = rightarg->sk_subtype;
+	if (righttype == InvalidOid)
+		righttype = opcintype;
+	optype = op->sk_subtype;
+	if (optype == InvalidOid)
+		optype = opcintype;
+
+	/*
+	 * If leftarg and rightarg match the types expected for the "op" scankey,
+	 * we can use its already-looked-up comparison function.
+	 */
+	if (lefttype == opcintype && righttype == optype)
+	{
+		*result = DatumGetBool(FunctionCall2(&op->sk_func,
+											 leftarg->sk_argument,
+											 rightarg->sk_argument));
+		return true;
+	}
+
+	/*
+	 * Otherwise, we need to go to the syscache to find the appropriate
+	 * operator.  (This cannot result in infinite recursion, since no
+	 * indexscan initiated by syscache lookup will use cross-data-type
+	 * operators.)
+	 */
+	cmp_op = get_opfamily_member(rel->rd_opfamily[leftarg->sk_attno - 1],
+								 lefttype,
+								 righttype,
+								 op->sk_strategy);
+	if (OidIsValid(cmp_op))
+	{
+		RegProcedure cmp_proc = get_opcode(cmp_op);
+
+		if (RegProcedureIsValid(cmp_proc))
+		{
+			*result = DatumGetBool(OidFunctionCall2(cmp_proc,
+													leftarg->sk_argument,
+													rightarg->sk_argument));
+			return true;
+		}
+	}
+
+	/* Can't make the comparison */
+	*result = false;			/* suppress compiler warnings */
+	return false;
+}
+
 /*
  * Mark a scankey as "required to continue the scan".
  *