Re: GIN improvements part 1: additional information - Mailing list pgsql-hackers

From Alexander Korotkov
Subject Re: GIN improvements part 1: additional information
Date
Msg-id CAPpHfdsYTRWuR5YkadvB_G=JiBRLsYRAL7CJkpPiz3AitvhD6g@mail.gmail.com
Whole thread Raw
In response to Re: GIN improvements part 1: additional information  (Heikki Linnakangas <hlinnakangas@vmware.com>)
Responses Re: GIN improvements part 1: additional information
List pgsql-hackers
On Fri, Jan 17, 2014 at 10:38 PM, Heikki Linnakangas <hlinnakangas@vmware.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.

pgsql-hackers by date:

Previous
From: Heikki Linnakangas
Date:
Subject: Re: GIN improvements part 1: additional information
Next
From: Tom Lane
Date:
Subject: Re: Add %z support to elog/ereport?