From f937e50b9252c667b22dd847e61b0ead15e6964f Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" Date: Thu, 13 Nov 2025 16:45:40 -0800 Subject: [PATCH v2] Improve comment and error handling in GetOperatorFromCompareType Despite the comment, we always ereport if we don't find a strategy number. Also we should avoid using uninitialized opfamily and opcintype values to build error messages. If get_opclass_method fails and get_opclass_opfamily_and_input_type doesn't, or vice versa, something is very wrong, since they both do the same syscache lookup by opclass oid. Since we elog for one, we can elog for the other. Similar reasoning lets us flip the order of the functions. This surfaces the error message one level, for easier top-to-bottom reading. Now we also have fewer paths through the function, so reasoning is easier. Finally we reduce nesting by inverting the conditional, treating it like a guard. Author: Paul A. Jungwirth --- src/backend/commands/indexcmds.c | 55 ++++++++++++++++---------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 5712fac3697..748ceeda650 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2439,8 +2439,7 @@ GetDefaultOpClass(Oid type_id, Oid am_id) * Finds an operator from a CompareType. This is used for temporal index * constraints (and other temporal features) to look up equality and overlaps * operators. We ask an opclass support function to translate from the - * compare type to the internal strategy numbers. If the function isn't - * defined or it gives no result, we set *strat to InvalidStrategy. + * compare type to the internal strategy numbers. */ void GetOperatorFromCompareType(Oid opclass, Oid rhstype, CompareType cmptype, @@ -2452,34 +2451,36 @@ GetOperatorFromCompareType(Oid opclass, Oid rhstype, CompareType cmptype, Assert(cmptype == COMPARE_EQ || cmptype == COMPARE_OVERLAP || cmptype == COMPARE_CONTAINED_BY); - amid = get_opclass_method(opclass); + /* + * Use the opclass to get the opfamily, opcintype, and access method. + * If any of this fails, quit early. + */ + if (!get_opclass_opfamily_and_input_type(opclass, &opfamily, &opcintype)) + elog(ERROR, "cache lookup failed for opclass %u", opclass); - *opid = InvalidOid; + amid = get_opclass_method(opclass); - if (get_opclass_opfamily_and_input_type(opclass, &opfamily, &opcintype)) - { - /* - * Ask the index AM to translate to its internal stratnum - */ - *strat = IndexAmTranslateCompareType(cmptype, amid, opfamily, true); - if (*strat == InvalidStrategy) - ereport(ERROR, - errcode(ERRCODE_UNDEFINED_OBJECT), - cmptype == COMPARE_EQ ? errmsg("could not identify an equality operator for type %s", format_type_be(opcintype)) : - cmptype == COMPARE_OVERLAP ? errmsg("could not identify an overlaps operator for type %s", format_type_be(opcintype)) : - cmptype == COMPARE_CONTAINED_BY ? errmsg("could not identify a contained-by operator for type %s", format_type_be(opcintype)) : 0, - errdetail("Could not translate compare type %d for operator family \"%s\" of access method \"%s\".", - cmptype, get_opfamily_name(opfamily, false), get_am_name(amid))); + /* + * Ask the index AM to translate to its internal stratnum + */ + *strat = IndexAmTranslateCompareType(cmptype, amid, opfamily, true); + if (*strat == InvalidStrategy) + ereport(ERROR, + errcode(ERRCODE_UNDEFINED_OBJECT), + cmptype == COMPARE_EQ ? errmsg("could not identify an equality operator for type %s", format_type_be(opcintype)) : + cmptype == COMPARE_OVERLAP ? errmsg("could not identify an overlaps operator for type %s", format_type_be(opcintype)) : + cmptype == COMPARE_CONTAINED_BY ? errmsg("could not identify a contained-by operator for type %s", format_type_be(opcintype)) : 0, + errdetail("Could not translate compare type %d for operator family \"%s\" of access method \"%s\".", + cmptype, get_opfamily_name(opfamily, false), get_am_name(amid))); - /* - * We parameterize rhstype so foreign keys can ask for a <@ operator - * whose rhs matches the aggregate function. For example range_agg - * returns anymultirange. - */ - if (!OidIsValid(rhstype)) - rhstype = opcintype; - *opid = get_opfamily_member(opfamily, opcintype, rhstype, *strat); - } + /* + * We parameterize rhstype so foreign keys can ask for a <@ operator + * whose rhs matches the aggregate function. For example range_agg + * returns anymultirange. + */ + if (!OidIsValid(rhstype)) + rhstype = opcintype; + *opid = get_opfamily_member(opfamily, opcintype, rhstype, *strat); if (!OidIsValid(*opid)) ereport(ERROR, -- 2.45.0