Re: Fix for gistchoose

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.)

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.

Do you want to have another go at it, or would you like me to try to
make it better?

regards, tom lane