The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
Hi, all!
It seems that as of now we have two sets of patches for this bug:
1. Tom Lane's: 0001-make-callbacks-ternary.patch and 0002-remove-calc-not-flag.patch
2. My: gin-gist-weight-patch-v4.diff
There was a quite long discussion above and I suppose that despite the difference both of them suit and will do the
necessaryfix.
So I decided to make a review of both Tom Lane's patches.
Both of them apply clean. Checks are sucessful. There are regression tests included and they cover the bug. Also I made
checkson my PgList database and I suppose the bug is indeed fixed.
For 0001-make-callbacks-ternary.patch
As it was mentioned in discussion, the issue was that in certain cases compare function of a single operand in a query
shouldgive undefined meaning "MAYBE" which should remain towards to the root of a tree. So the patch in my opinion
adressesthe problem in a right way.
Possible dangers of changed callback from binary to ternary is that any side modules which still use binary interface
willget warnings on compile and will need minor modifications of code to comply with new interface. I checked it with
RUMindex and indeed get warnings on compile. In discussion above it was noted that anyway there is no way to get right
resultsin tsearch with NOT without modification of this so I'd recommend committing patch 0001.
For 0002-remove-calc-not-flag.patch
The patch changes the behavior which is now considered default. This is true in RUM module and maybe in some other
tsearchside modules. Applying the patch can make code more beautiful but possibly will not give some performance gain
andbug is anyway fixed by patch 0001.
Overall I'd recommend patch 0001-make-callbacks-ternary.patch and close the issue.
--
Best regards,
Pavel Borisov
Postgres Professional: http://postgrespro.com
The new status of this patch is: Ready for Committer
