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
