On Fri, 21 Nov 2025 at 05:45, Paul A Jungwirth <[email protected]> wrote: > Thanks for the review! Here is a patch with your suggestions incorporated.
I had a look at this. I agree the code could be made simpler, but I don't see any window for "potentially using uninitialized Oids to build error messages". I think you must be talking about the final ERROR message using opfamily and opcintype, but it seems to me like the call to get_opclass_method() would ERROR if the opclass couldn't be found and there's no window for the opclass to be removed before the call to get_opclass_opfamily_and_input_type() as we don't process catcache invalidations in between. That makes me think there's no live issue here, so it's more just about a cleanup and simplification. I split your patch into two and wrote a comment to explain about ERRORs are raised on failed lookups. We should likely fix that in v18 since the comment is misleading, but for 0002, since nothing seems broken, then it seems safer just to do that one in master. What do you think? David
From 6e158adc3536134ed5ea7de1967eecf6622630da Mon Sep 17 00:00:00 2001 From: David Rowley <[email protected]> Date: Mon, 5 Jan 2026 15:14:29 +1300 Subject: [PATCH v3 1/2] Fix misleading comment for GetOperatorFromCompareType The comment claimed *strat got set to InvalidStrategy when the function lookup fails. This isn't true, an ERROR is raised when that happens. Author: Paul A Jungwirth <[email protected]> Discussion: https://postgr.es/m/ca+renyxorjlacp_nhqequf2w+zcoy2q5kpqcfg05vqvyzr8...@mail.gmail.com Backpatch-through: 18 --- src/backend/commands/indexcmds.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 755dc00c86f..0a7b6f18fa2 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2460,8 +2460,8 @@ 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. Raises ERROR on search + * failure. */ void GetOperatorFromCompareType(Oid opclass, Oid rhstype, CompareType cmptype, -- 2.51.0
From 9552f30c710f5c09af90dd6712b9ff886d2ee318 Mon Sep 17 00:00:00 2001 From: David Rowley <[email protected]> Date: Mon, 5 Jan 2026 15:19:01 +1300 Subject: [PATCH v3 2/2] Simplify GetOperatorFromCompareType() code The old code would set *opid = InvalidOid to determine if the get_opclass_opfamily_and_input_type() worked or not. This means more moving parts that what's really needed here. Let's just fail immediately if the get_opclass_opfamily_and_input_type() lookup fails. Author: Paul A Jungwirth <[email protected]> Reviewed-by: Neil Chen <[email protected]> Discussion: https://postgr.es/m/ca+renyxorjlacp_nhqequf2w+zcoy2q5kpqcfg05vqvyzr8...@mail.gmail.com --- src/backend/commands/indexcmds.c | 52 +++++++++++++++++--------------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 0a7b6f18fa2..08c86cc163c 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2473,34 +2473,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.51.0
