Re: Allowing extensions to supply operator-/function-specific info
От | Paul Ramsey |
---|---|
Тема | Re: Allowing extensions to supply operator-/function-specific info |
Дата | |
Msg-id | 14E63B07-1751-4CE6-9126-4D237B9C085A@cleverelephant.ca обсуждение исходный текст |
Ответ на | Re: Allowing extensions to supply operator-/function-specific info (Tom Lane <tgl@sss.pgh.pa.us>) |
Ответы |
Re: Allowing extensions to supply operator-/function-specific info
|
Список | pgsql-hackers |
> 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.
В списке pgsql-hackers по дате отправления: