Re: Fix for gistchoose

On Thu, Aug 30, 2012 at 3:27 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Thu, Aug 30, 2012 at 1:27 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Should we backpatch that?
>
>> Arguably, yes. Does the patch look sane to you?
>
> I was afraid you'd ask that.
>
> [ studies code for awhile ... ]
>
> I think this fixes the bug, but the function could really do with slightly
> more invasive code adjustments for clarity. For instance, we could get
> rid of the "sum_grow = 1" kluge if we removed the "&& sum_grow" from the
> outer for statement (where it's pretty damn ugly/surprising anyway ---
> I dislike for loops that look like they're just running a counter when
> they're doing other things too) and instead put something like this at
> the bottom of the outer loop:
>
> /*
> * If we examined all the columns and there is zero penalty for
> * all of them, we can stop examining tuples; this is a good one
> * to insert the new key into.
> */
> if (sum_grow == 0 && j == r->rd_att->natts)
> break;
>
> I'm not thrilled with the added comments either; they need copy-editing
> and they randomly fail to fit in an 80-column window. (pgindent will
> have something to say about that later for some of them, but I think it
> doesn't reformat comments that start at the left margin.)

Sorry. I must have had my window sized wrong. At any rate, as far as
the GiST code goes anyway, I'm inclined to think that even mis-spelled
comments are an improvement over the status quo.

> BTW, I think the real issue with the bug is not so much that we're
> resetting anything as that we may be initializing which_grow[j] for
> the next index column for the first time. Note that the function's
> startup code only bothers to initialize which_grow[0] (although it
> does so in a less than legible fashion). The rest have to get filled
> as the loop proceeds.

I noticed all that, but didn't feel like putting in the effort to make
it better. I would have been happy to have someone else pick up the
patch, but as it had been languishing I thought it would be better to
get it committed more or less as it was than to wait for someone to
have time to make it beautiful. If you want to hack on it more that's
fine with me. I kind of wonder if we ought to rename the variables,
and maybe turn sum_grow into a boolean. But I'm not really eager to
go crazy if this is something we have to back-patch.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company