Hi! On Fri, Dec 8, 2017 at 11:21 AM, Andrey Borodin <x4...@yandex-team.ru> wrote:
> The following review has been posted through the commitfest application: > make installcheck-world: tested, passed > Implements feature: tested, passed > Spec compliant: tested, passed > Documentation: tested, passed > > Hi! I've reviewed the patch. > Patch works as expected, documented and tested. > Thank you for reviewing this. > The code does not use LL_COORD and UR_COORD used all around. But the code > still is easily readable. > > LL_COORD and UR_COORD are always combined with Min or Max function, so LL > (Lower Left) and UR (Upper Right) are somewhat irrelevant names... > BTW if we swap implementation of these macro-funcs and fix cube_out, > almost all tests pass again. This is evidence of good compatibility and > shows that compatibility overhead is added everywhere. > Yes, the thing is that we change behavior of existing ~> operator. In general, this is not good idea because it could affect existing users whose already use this operator. Typically in such situation, we could leave existing operator as is, and invent new operator with new behavior. However, in this particular case, I think there are reasons to make an exception to the rules. The reasons are following: 1) The ~> operator was designed especially for knn gist. 2) Knn gist support for current behavior is broken by design and can't be fixed. Most we can do to fix existing ~> operator behavior as is to drop knn gist support. But then ~> operator would be left useless. 3) It doesn't seems that ~> operator have many users yet, because an error wasn't reported during whole release cycle. I hope these reasons justify altering behavior of existing operator as an exception to the rules. Another question is whether we should backpatch it. But I think we could leave this decision to committer. I think that this patch is ready for committer. > Good. ------ Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
