Обсуждение: Fix GetOperatorFromCompareType

Hi Hackers,

I found a few problems with GetOperatorFromCompareType and fixed them here.

First of all, the comment was out of date: we never return
InvalidStrategy; instead we ereport.

Second, we were potentially using uninitialized Oids to build error
messages (if get_opclass_opfamily_and_input_type failed). If that
function fails, we should just die. In fact since get_opclass_method
just succeeded, which makes the same lookup, how could it ever fail? I
don't think we need to try very hard to build a fancy message.

Failing right away simplifies the logic, because we only reach the
bottom of the function one way. And I think we can make things ever
clearer by inverting the conditional, so it acts like a guard, and we
can avoid some nesting.

Based on acbc9beaae.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

On Sun, Nov 16, 2025 at 11:18 PM Neil Chen <carpenter.nail.cz@gmail.com> wrote:
> After your patch changes, the line '*opid = InvalidOid;' seems removable.
>
> Also, if the second validation check of opclass after 'get_opclass_method' feels a bit odd, moving
'get_opclass_opfamily_and_input_type'to the very top would work -- purely for visual clarity. ：） 

Thanks for the review! Here is a patch with your suggestions incorporated.

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

On Fri, 21 Nov 2025 at 05:45, Paul A Jungwirth
<pj@illuminatedcomputing.com> 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

On Sun, Jan 4, 2026 at 6:46 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Fri, 21 Nov 2025 at 05:45, Paul A Jungwirth
> <pj@illuminatedcomputing.com> 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.

Thanks for taking a look! I agree it should never happen, although it
seems easy for future code to introduce a problem, so I think cleaning
it up is also preventative.

> 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?

This split makes sense to me and seems fine.

If you want to be very strict, I think this should be a semicolon, not a comma:

    The comment claimed *strat got set to InvalidStrategy when the function
    lookup fails.  This isn't true, an ERROR is raised when that happens.

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

On Tue, 6 Jan 2026 at 06:34, Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
> This split makes sense to me and seems fine.

Thanks. Done.

David