Re: Small patch for GiST: move childoffnum to child

On Tue, Jun 28, 2011 at 10:21 AM, Alexander Korotkov
<aekorotkov(at)gmail(dot)com> wrote:
> Actually, there is no more direct need of this patch because I've rewrote
> insert function for fast build. But there are still two points for having
> this changes:
> 1) As it was noted before, it simplifies code a bit.
> 2) It would be better if childoffnum have the same semantics in regular
> insert and fastbuild insert.

Hi Alexander,

I've looked at your patch and have done a "partial" review. It
applies cleanly and makes without warnings, and passes make check plus
some additional testing I've done (inserting lots of stuff into
regression's test_tsvector table, in parallel, with the gist index in
place) under --enable-cassert. I repeated that test without
--enable-cassert, and saw no degradation in performance over unpatched
code. No changes to documentation or "make check" code should be
needed. The formatting looks OK.

My concern is that I am unable to prove to myself simply by reading
the code that the 24 line chunk deleted from gistFindPath (near ***
919,947 ****) are no longer needed. My familiarity with the gist code
is low enough that it is not surprising that I cannot prove this to
myself from first principles. I have no reason to believe it is not
correct, it is just that I can't convince myself that it is correct.

So I tried provoking situations where this surrounding code section
would get executed, both patched and unpatched. I have been unable to
do so--apparently this code is for an incredibly obscure situation
which I can't induce at will.

I would love to use this as an opportunity to study the gist code
until I can convince myself this patch is correct, but I'm afraid I
won't be able to do that promptly, or for the remainder of this commit
fest.

Since Heikki has already looked at this patch, perhaps he can provide
the assurance that I cannot, or another reviewer can.

Sorry I couldn't do a more thorough review,

Jeff