Re: GIN improvements part 1: additional information

On Fri, Jan 17, 2014 at 10:38 PM, Heikki Linnakangas <
hlinnakangas(at)vmware(dot)com> wrote:

> On 01/17/2014 01:05 PM, Alexander Korotkov wrote:
>
>> Seems to be fixed in the attached version of patch.
>> Another improvement in this version: only left-most segments and further
>> are re-encoded. Left part of page are left untouched.
>>
>
> I'm looking into this now. A few quick notes:
>
> * Even when you don't re-encode the whole page, you still WAL-log the
> whole page. While correct, it'd be a pretty obvious optimization to only
> WAL-log the modified part.
>

Oh, I overlooked it. I wrote code in ginRedoInsertData which finds
appropriate place fro data but didn't notice that I still wrote whole page
to xlog record.

> * When new items are appended to the end of the page so that only the last
> existing compressed segment is re-encoded, and the page has to be split,
> the items aren't divided 50/50 on the pages. The loop that moves segments
> destined for the left page to the right won't move any existing, untouched,
> segments.
>

I think this loop will move one segment. However, it's too small.

> ! /*
>> ! * Didn't fit uncompressed. We'll have to encode them. Check if
>> both
>> ! * new items and uncompressed items can be placed starting from
>> last
>> ! * segment of page. Then re-encode only last segment of page.
>> ! */
>> ! minNewItem = newItems[0];
>> ! if (nolduncompressed == 0 &&
>> ! ginCompareItemPointers(&olduncompressed[0],
>> &minNewItem) < 0)
>> ! minNewItem = olduncompressed[0];
>>
>
> That looks wrong. If I'm understanding it right, it's trying to do
> minNewItem = Min(newItems[0], olduncompressed[0]). The test should be
> "nolduncompressed > 0 && ..."
>

Yes, definitely a bug.

> No need to send a new patch, I'll just fix those while reviewing...

Ok, thanks!

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