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