Re: Allowing extensions to supply operator-/function-specific info

Paul Ramsey <pramsey(at)cleverelephant(dot)ca> writes:
> On Mar 5, 2019, at 3:26 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Hm, I think your addition of this bit is wrong:
>>
>> + /*
>> + * Arguments were swapped to put the index value on the
>> + * left, so we need the commutated operator for
>> + * the OpExpr
>> + */
>> + if (swapped)
>> + {
>> + oproid = get_commutator(oproid);
>> + if (!OidIsValid(oproid))
>> PG_RETURN_POINTER((Node *)NULL);
>> + }
>>
>> We already did the operator lookup with the argument types in the desired
>> order, so this is introducing an extra swap. The only reason it appears
>> to work, I suspect, is that all your index operators are self-commutators.

> I was getting regression failures until I re-swapped the operator…
> SELEcT * FROM foobar WHERE ST_Within(ConstA, VarB)

Ah ... so the real problem here is that *not* all of your functions
treat their first two inputs alike, and the hypothetical future
improvement I commented about is needed right now. I should've
looked more closely at the strategies in your table; then I would've
realized the patch as I proposed it didn't work.

But this code isn't right either. I'm surprised you're not getting
crashes --- perhaps there aren't cases where the first and second args
are of incompatible types? Also, it's certainly wrong to be doing this
sort of swap in only one of the two code paths.

There's more than one way you could handle this, but the way that
I was vaguely imagining was to have two strategy entries in each
IndexableFunction entry, one to apply if the first function argument
is the indexable one, and the other to apply if the second function
argument is the indexable one. If you leave the operator lookup as
I had it (using the already-swapped data types) then you'd have to
make sure that the latter set of strategy entries are written as
if the arguments get swapped before selecting the strategy, which
would be confusing perhaps :-( --- for instance, st_within would
use RTContainedByStrategyNumber in the first case but
RTContainsStrategyNumber in the second. But otherwise you need the
separate get_commutator step, which seems like one more catalog lookup
than you really need.

> I feel OK about it, if for no other reason than it passes all the tests :)

Then you're at least missing adequate tests for the 3-arg functions...
3 args with the index column second will not work as this stands.

regards, tom lane