On Fri, Apr 8, 2016 at 10:04 AM, Constantin S. Pan <kvapen@gmail.com> wrote:
> Here is a new version of the patch, which:
>
> 1. Fixes some minor stylistic issues.
>
> 2. Uses binaryheap (instead of a custom ugly stack) for merging.
I think we need to push this patch out to 9.7. This code has had a
little review here and there from various people, but clearly not
enough to push it into the tree at the very last minute. I don't
think we even have enough discussion to conclude that things like the
gin_shared_mem GUCs are good ideas rather than bad ones.
Also, I personally find this code to be extremely low on comments.
There's barely any explanation of what the overall methodology is
here. Most of the functions do not have a header comment explaining
what they do. The code hasn't been run through pgindent. There's no
check to see whether the computation that will be done inside the GIN
index is parallel-safe; what if it's an expression index on an unsafe
function? Opening the heap and index with no lock in the worker is
wrong; the worker should use the same lock mode as the leader.
That's just on a quick read-through; I'm sure there's more. I'm going
to move this to the next CommitFest. Hopefully someone will volunteer
to do some serious review of this, because the feature sounds cool.
Possibly that person will even be me. But it will not be today.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company