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

> On Mar 5, 2019, at 3:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Paul Ramsey <pramsey@cleverelephant.ca> writes:
>> On Mar 5, 2019, at 3:26 PM, Tom Lane <tgl@sss.pgh.pa.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.

Some of the operators are indifferent to order (&&,  overlaps) and others are not (@, within) (~, contains).

The 3-arg functions fortunately all have && strategies.

The types on either side of the operators are always the same (geometry && geometry), ST_Intersects(geometry, geometry).

I could simply be getting a free pass from the simplicity of my setup?

P.