Re: gistchoose vs. bloat

On Mon, Oct 1, 2012 at 5:15 AM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:

> On Tue, 2012-09-04 at 19:21 +0400, Alexander Korotkov wrote:
>
> > New version of patch is attached. Parameter "randomization" was
> > introduced. It controls whether to randomize choose. Choose algorithm
> > was rewritten.
> >
> Review comments:
>
> 1. Comment above while loop in gistRelocateBuildBuffersOnSplit needs to
> be updated.
>

Actually, I didn't realize what exact comment you expect. Check if added
commend meets you expectations.

>
> 2. Typo in two places: "if randomization id required".
>
> 3. In gistRelocateBuildBuffersOnSplit, shouldn't that be:
> splitPageInfo = &relocationBuffersInfos[bufferIndex];
> not:
> splitPageInfo = &relocationBuffersInfos[i];
>

Fixed.

> 4. It looks like the randomization is happening while trying to compare
> the penalties. I think it may be more readable to separate those two
> steps; e.g.
>
> /* create a mapping whether randomization is on or not */
> for (i = FirstOffsetNumber; i <= maxoff; i = OffsetNumberNext(i))
> offsets[i - FirstOffsetNumber] = i;
>
> if (randomization)
> /* randomize offsets array */
>
> for (i = 0; i < maxoff; i++)
> {
> offset = offsets[i];
> ...
> }
>
> That's just an idea; if you think it's more readable as-is (or if I am
> misunderstanding) then let me know.
>

Actually, current implementation comes from idea of creating possible less
overhead when randomization is off. I'll try to measure overhead in worst
case. If it is low enough then you proposal looks reasonable to me.

------
With best regards,
Alexander Korotkov.