Re: [PATCH] fix GIN index search sometimes losing results

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 necessary fix.
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 checks on 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 should give undefined meaning "MAYBE" which should remain towards to the root of a tree. So the patch in my opinion adresses the problem in a right way.

Possible dangers of changed callback from binary to ternary is that any side modules which still use binary interface will get warnings on compile and will need minor modifications of code to comply with new interface. I checked it with RUM index and indeed get warnings on compile. In discussion above it was noted that anyway there is no way to get right results in 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 tsearch side modules. Applying the patch can make code more beautiful but possibly will not give some performance gain and bug 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