On Mon, 2012-06-18 at 15:12 +0400, Alexander Korotkov wrote:
> Hackers,
>
>
> While experimenting with gistchoose I achieve interesting results
> about relation of gistchoose behaviour and gist index bloat.
...
>
> Current implementation of gistchoose select first index tuple which
> have minimal penalty. It is possible for several tuples to have same
> minimal penalty. I've created simple patch which selects random from
> them. I then I've following results for same testcase.
>
I took a look at this patch. The surrounding code is pretty messy (not
necessarily because of your patch). A few comments would go a long way.
The 'which_grow' array is initialized as it goes, first using pointer
notations ("*which_grows = -1.0") and then using subscript notation. As
far as I can tell, the first r->rd_att->natts of the array (the only
elements that matter) need to be written the first time through anyway.
Why not just replace "which_grow[j] < 0" with "i == FirstOffsetNumber"
and add a comment that we're initializing the penalties with the first
index tuple?
The 'sum_grow' didn't make any sense, thank you for getting rid of that.
Also, we should document that the earlier attributes always take
precedence, which is why we break out of the inner loop as soon as we
encounter an attribute with a higher penalty.
Please add a comment indicating why you are randomly choosing among the
equal penalties.
I think that there might be a problem with the logic, as well. Let's say
you have two attributes and there are two index tuples, it1 and it2;
with penalties [10,10] and [10,100] respectively. The second time
through the outer loop, with i = 2, you might (P=0.5) assign 2 to the
'which' variable in the first iteration of the inner loop, before it
realizes that it2 actually has a higher penalty. I think you need to
finish out the inner loop and have a flag that indicates that all
attributes are equal before you do the probabilistic replacement.
Also, I think you should use random() rather than rand().
Regards,Jeff Davis