Skip site navigation (1) Skip section navigation (2)

Re: Small patch for GiST: move childoffnum to child

From: Jeff Janes <jeff(dot)janes(at)gmail(dot)com>
To: Alexander Korotkov <aekorotkov(at)gmail(dot)com>
Cc: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Small patch for GiST: move childoffnum to child
Date: 2011-06-30 04:50:57
Message-ID: BANLkTinjmZ28MyYPLYSWjje=-OcgMi9L3A@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
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

In response to

Responses

pgsql-hackers by date

Next:From: Casey HavenorDate: 2011-06-30 05:22:03
Subject: Re: Patch file questions?
Previous:From: Greg SmithDate: 2011-06-30 04:35:49
Subject: Re: pgbench--new transaction type

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group