Improve index AMs' opclass validation procedures.

The amvalidate functions added in commit 65c5fcd353 were on the crude side. Improve them in a few ways: * Perform signature checking for operators and support functions. * Apply more thorough checks for missing operators and functions, where possible. * Instead of reporting problems as ERRORs, report most problems as INFO messages and make the amvalidate function return FALSE. This allows more than one problem to be discovered per run. * Report object names rather than OIDs, and work a bit harder on making the messages understandable. Also, remove a few more opr_sanity regression test queries that are now superseded by the amvalidate checks.
2025-11-07 19:06:32 +03:00 · 2016-01-21 19:47:15 -05:00
parent b99551832e
commit be44ed27b8
13 changed files with 1414 additions and 771 deletions
--- a/src/backend/access/hash/hashvalidate.c
+++ b/src/backend/access/hash/hashvalidate.c
@@ -13,16 +13,24 @@
 */
 #include "postgres.h"

+#include "access/amvalidate.h"
 #include "access/hash.h"
 #include "access/htup_details.h"
 #include "catalog/pg_amop.h"
 #include "catalog/pg_amproc.h"
 #include "catalog/pg_opclass.h"
+#include "catalog/pg_opfamily.h"
+#include "catalog/pg_proc.h"
+#include "catalog/pg_type.h"
+#include "parser/parse_coerce.h"
 #include "utils/builtins.h"
-#include "utils/catcache.h"
+#include "utils/fmgroids.h"
 #include "utils/syscache.h"


+static bool check_hash_func_signature(Oid funcid, Oid restype, Oid argtype);
+
+
 /*
 * Validator for a hash opclass.
 *
@@ -30,23 +38,26 @@
 * redundant when checking each opclass in a family.  But they don't run long
 * enough to be much of a problem, so we accept the duplication rather than
 * complicate the amvalidate API.
- *
- * Some of the code here relies on the fact that hash has only one operator
- * strategy and support function; we don't have to check for incomplete sets.
 */
 bool
 hashvalidate(Oid opclassoid)
 {
+	bool		result = true;
 	HeapTuple	classtup;
 	Form_pg_opclass classform;
 	Oid			opfamilyoid;
 	Oid			opcintype;
-	int			numclassops;
-	int32		classfuncbits;
+	char	   *opclassname;
+	HeapTuple	familytup;
+	Form_pg_opfamily familyform;
+	char	   *opfamilyname;
 	CatCList   *proclist,
 			   *oprlist;
-	int			i,
-				j;
+	List	   *grouplist;
+	OpFamilyOpFuncGroup *opclassgroup;
+	List	   *hashabletypes = NIL;
+	int			i;
+	ListCell   *lc;

 	/* Fetch opclass information */
 	classtup = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclassoid));
@@ -56,102 +67,246 @@ hashvalidate(Oid opclassoid)

 	opfamilyoid = classform->opcfamily;
 	opcintype = classform->opcintype;
+	opclassname = NameStr(classform->opcname);

-	ReleaseSysCache(classtup);
+	/* Fetch opfamily information */
+	familytup = SearchSysCache1(OPFAMILYOID, ObjectIdGetDatum(opfamilyoid));
+	if (!HeapTupleIsValid(familytup))
+		elog(ERROR, "cache lookup failed for operator family %u", opfamilyoid);
+	familyform = (Form_pg_opfamily) GETSTRUCT(familytup);
+
+	opfamilyname = NameStr(familyform->opfname);

 	/* Fetch all operators and support functions of the opfamily */
 	oprlist = SearchSysCacheList1(AMOPSTRATEGY, ObjectIdGetDatum(opfamilyoid));
 	proclist = SearchSysCacheList1(AMPROCNUM, ObjectIdGetDatum(opfamilyoid));

-	/* We'll track the ops and functions belonging to the named opclass */
-	numclassops = 0;
-	classfuncbits = 0;
-
-	/* Check support functions */
+	/* Check individual support functions */
 	for (i = 0; i < proclist->n_members; i++)
 	{
 		HeapTuple	proctup = &proclist->members[i]->tuple;
 		Form_pg_amproc procform = (Form_pg_amproc) GETSTRUCT(proctup);

-		/* Check that only allowed procedure numbers exist */
-		if (procform->amprocnum != HASHPROC)
-			ereport(ERROR,
+		/*
+		 * All hash functions should be registered with matching left/right
+		 * types
+		 */
+		if (procform->amproclefttype != procform->amprocrighttype)
+		{
+			ereport(INFO,
 					(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-					 errmsg("hash opfamily %u contains invalid support number %d for procedure %u",
-							opfamilyoid,
-							procform->amprocnum, procform->amproc)));
+					 errmsg("hash opfamily %s contains support procedure %s with cross-type registration",
+							opfamilyname,
+							format_procedure(procform->amproc))));
+			result = false;
+		}

-		/* Remember functions that are specifically for the named opclass */
-		if (procform->amproclefttype == opcintype &&
-			procform->amprocrighttype == opcintype)
-			classfuncbits |= (1 << procform->amprocnum);
+		/* Check procedure numbers and function signatures */
+		switch (procform->amprocnum)
+		{
+			case HASHPROC:
+				if (!check_hash_func_signature(procform->amproc, INT4OID,
+											   procform->amproclefttype))
+				{
+					ereport(INFO,
+							(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+							 errmsg("hash opfamily %s contains function %s with wrong signature for support number %d",
+									opfamilyname,
+									format_procedure(procform->amproc),
+									procform->amprocnum)));
+					result = false;
+				}
+				else
+				{
+					/* Remember which types we can hash */
+					hashabletypes =
+						list_append_unique_oid(hashabletypes,
+											   procform->amproclefttype);
+				}
+				break;
+			default:
+				ereport(INFO,
+						(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+						 errmsg("hash opfamily %s contains function %s with invalid support number %d",
+								opfamilyname,
+								format_procedure(procform->amproc),
+								procform->amprocnum)));
+				result = false;
+				break;
+		}
 	}

-	/* Check operators */
+	/* Check individual operators */
 	for (i = 0; i < oprlist->n_members; i++)
 	{
 		HeapTuple	oprtup = &oprlist->members[i]->tuple;
 		Form_pg_amop oprform = (Form_pg_amop) GETSTRUCT(oprtup);
-		bool		leftFound = false,
-					rightFound = false;

 		/* Check that only allowed strategy numbers exist */
 		if (oprform->amopstrategy < 1 ||
 			oprform->amopstrategy > HTMaxStrategyNumber)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-					 errmsg("hash opfamily %u contains invalid strategy number %d for operator %u",
-							opfamilyoid,
-							oprform->amopstrategy, oprform->amopopr)));
-
-		/*
-		 * There should be relevant hash procedures for each operator
-		 */
-		for (j = 0; j < proclist->n_members; j++)
 		{
-			HeapTuple	proctup = &proclist->members[j]->tuple;
-			Form_pg_amproc procform = (Form_pg_amproc) GETSTRUCT(proctup);
-
-			if (procform->amproclefttype == oprform->amoplefttype)
-				leftFound = true;
-			if (procform->amproclefttype == oprform->amoprighttype)
-				rightFound = true;
-		}
-
-		if (!leftFound || !rightFound)
-			ereport(ERROR,
+			ereport(INFO,
 					(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-			errmsg("hash opfamily %u lacks support function for operator %u",
-				   opfamilyoid, oprform->amopopr)));
+					 errmsg("hash opfamily %s contains operator %s with invalid strategy number %d",
+							opfamilyname,
+							format_operator(oprform->amopopr),
+							oprform->amopstrategy)));
+			result = false;
+		}

 		/* hash doesn't support ORDER BY operators */
 		if (oprform->amoppurpose != AMOP_SEARCH ||
 			OidIsValid(oprform->amopsortfamily))
-			ereport(ERROR,
+		{
+			ereport(INFO,
 					(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-					 errmsg("hash opfamily %u contains invalid ORDER BY specification for operator %u",
-							opfamilyoid, oprform->amopopr)));
+					 errmsg("hash opfamily %s contains invalid ORDER BY specification for operator %s",
+							opfamilyname,
+							format_operator(oprform->amopopr))));
+			result = false;
+		}

-		/* Count operators that are specifically for the named opclass */
-		if (oprform->amoplefttype == opcintype &&
-			oprform->amoprighttype == opcintype)
-			numclassops++;
+		/* Check operator signature --- same for all hash strategies */
+		if (!check_amop_signature(oprform->amopopr, BOOLOID,
+								  oprform->amoplefttype,
+								  oprform->amoprighttype))
+		{
+			ereport(INFO,
+					(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+					 errmsg("hash opfamily %s contains operator %s with wrong signature",
+							opfamilyname,
+							format_operator(oprform->amopopr))));
+			result = false;
+		}
+
+		/* There should be relevant hash procedures for each datatype */
+		if (!list_member_oid(hashabletypes, oprform->amoplefttype) ||
+			!list_member_oid(hashabletypes, oprform->amoprighttype))
+		{
+			ereport(INFO,
+					(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+			errmsg("hash opfamily %s lacks support function for operator %s",
+				   opfamilyname,
+				   format_operator(oprform->amopopr))));
+			result = false;
+		}
 	}

-	/* Check that the named opclass is complete */
-	if (numclassops != HTMaxStrategyNumber)
-		ereport(ERROR,
+	/* Now check for inconsistent groups of operators/functions */
+	grouplist = identify_opfamily_groups(oprlist, proclist);
+	opclassgroup = NULL;
+	foreach(lc, grouplist)
+	{
+		OpFamilyOpFuncGroup *thisgroup = (OpFamilyOpFuncGroup *) lfirst(lc);
+
+		/* Remember the group exactly matching the test opclass */
+		if (thisgroup->lefttype == opcintype &&
+			thisgroup->righttype == opcintype)
+			opclassgroup = thisgroup;
+
+		/*
+		 * Complain if there seems to be an incomplete set of operators for
+		 * this datatype pair (implying that we have a hash function but no
+		 * operator).
+		 */
+		if (thisgroup->operatorset != (1 << HTEqualStrategyNumber))
+		{
+			ereport(INFO,
+					(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+					 errmsg("hash opfamily %s is missing operator(s) for types %s and %s",
+							opfamilyname,
+							format_type_be(thisgroup->lefttype),
+							format_type_be(thisgroup->righttype))));
+			result = false;
+		}
+	}
+
+	/* Check that the originally-named opclass is supported */
+	/* (if group is there, we already checked it adequately above) */
+	if (!opclassgroup)
+	{
+		ereport(INFO,
 				(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-				 errmsg("hash opclass %u is missing operator(s)",
-						opclassoid)));
-	if ((classfuncbits & (1 << HASHPROC)) == 0)
-		ereport(ERROR,
+				 errmsg("hash opclass %s is missing operator(s)",
+						opclassname)));
+		result = false;
+	}
+
+	/*
+	 * Complain if the opfamily doesn't have entries for all possible
+	 * combinations of its supported datatypes.  While missing cross-type
+	 * operators are not fatal, it seems reasonable to insist that all
+	 * built-in hash opfamilies be complete.
+	 */
+	if (list_length(grouplist) !=
+		list_length(hashabletypes) * list_length(hashabletypes))
+	{
+		ereport(INFO,
 				(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-			   errmsg("hash opclass %u is missing required support function",
-					  opclassoid)));
+				 errmsg("hash opfamily %s is missing cross-type operator(s)",
+						opfamilyname)));
+		result = false;
+	}

 	ReleaseCatCacheList(proclist);
 	ReleaseCatCacheList(oprlist);
+	ReleaseSysCache(familytup);
+	ReleaseSysCache(classtup);

-	return true;
+	return result;
+}
+
+
+/*
+ * We need a custom version of check_amproc_signature because of assorted
+ * hacks in the core hash opclass definitions.
+ */
+static bool
+check_hash_func_signature(Oid funcid, Oid restype, Oid argtype)
+{
+	bool		result = true;
+	HeapTuple	tp;
+	Form_pg_proc procform;
+
+	tp = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
+	if (!HeapTupleIsValid(tp))
+		elog(ERROR, "cache lookup failed for function %u", funcid);
+	procform = (Form_pg_proc) GETSTRUCT(tp);
+
+	if (procform->prorettype != restype || procform->proretset ||
+		procform->pronargs != 1)
+		result = false;
+
+	if (!IsBinaryCoercible(argtype, procform->proargtypes.values[0]))
+	{
+		/*
+		 * Some of the built-in hash opclasses cheat by using hash functions
+		 * that are different from but physically compatible with the opclass
+		 * datatype.  In some of these cases, even a "binary coercible" check
+		 * fails because there's no relevant cast.  For the moment, fix it by
+		 * having a whitelist of allowed cases.  Test the specific function
+		 * identity, not just its input type, because hashvarlena() takes
+		 * INTERNAL and allowing any such function seems too scary.
+		 */
+		if (funcid == F_HASHINT4 &&
+			(argtype == DATEOID ||
+			 argtype == ABSTIMEOID || argtype == RELTIMEOID ||
+			 argtype == XIDOID || argtype == CIDOID))
+			 /* okay, allowed use of hashint4() */ ;
+		else if (funcid == F_TIMESTAMP_HASH &&
+				 argtype == TIMESTAMPTZOID)
+			 /* okay, allowed use of timestamp_hash() */ ;
+		else if (funcid == F_HASHCHAR &&
+				 argtype == BOOLOID)
+			 /* okay, allowed use of hashchar() */ ;
+		else if (funcid == F_HASHVARLENA &&
+				 argtype == BYTEAOID)
+			 /* okay, allowed use of hashvarlena() */ ;
+		else
+			result = false;
+	}
+
+	ReleaseSysCache(tp);
+	return result;
 }