Message-ID: <52D9789E.3030202@vmware.com>
Date: Fri, 17 Jan 2014 20:38:22 +0200
From: Heikki Linnakangas <hlinnakangas@vmware.com>
Organization: VMware
User-Agent: Mozilla/5.0 (X11; Linux x86_64;
 rv:17.0) Gecko/20131103 Icedove/17.0.10
MIME-Version: 1.0
To: Alexander Korotkov <aekorotkov@gmail.com>
CC: Tomas Vondra <tv@fuzzy.cz>, pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: GIN improvements part 1: additional information
References: 
 <CAPpHfdutJEC9+6vWX579afZKzdHKCT2wzDkAEom1XmiXEwF3pw@mail.gmail.com>
 <5298E834.4090408@vmware.com>
 <CAPpHfdvP3G2k04tpCyEA0mAd2e8xOQyuj=2wwAj0UVhB1_oe+g@mail.gmail.com>
 <52A58AD5.6030808@vmware.com>
 <CAPpHfdvKC9HE_D+BYBUpxqyWSE4UD_=ykZgfSxkAv4P3LBFQjg@mail.gmail.com>
 <52A62771.3070800@vmware.com>
 <CAPpHfdu2nXxxNCmtMBBiS+3CRdgF8AHCLt2w9Z4s2kKisJSPuA@mail.gmail.com>
 <52AEE445.1060206@vmware.com>
 <CAPpHfdvK3eZr8iQJtp9ax9k80MKRKkPKgqcc0SB1bn+_KVq-Xg@mail.gmail.com>
 <52AF837C.3040201@vmware.com> <52B2F5B0.1030809@vmware.com>
 <52B49C34.8060300@vmware.com>
 <CA+HiwqGO91OEE0ZBa0q2RqFEKm5dy5VzXrf7uoYObw9BCKSEVA@mail.gmail.com>
 <CAPpHfduBDxQ7Tw5bZCCgxmJzNHDRG6eELAuseuCEvKt=dw0Yfw@mail.gmail.com>
 <52D0A947.6050706@fuzzy.cz>
 <CAPpHfdskbPbjWYJhkd-FkZCKEUZd03PRAw05NyT0Hd-jxWOyfg@mail.gmail.com>
 <52D478F2.6070005@fuzzy.cz> <52D5E1BC.3000208@fuzzy.cz>
 <CAPpHfdv9Pheu5atEEUk75f_S1nf6vCoRqge2yUQ1v3xgZ4UP3g@mail.gmail.com>
 <CAPpHfds0A03zwmYdhKUg6s01Vq1p30n01=3c-tct264QTpV--Q@mail.gmail
 .com>
In-Reply-To: 
 <CAPpHfds0A03zwmYdhKUg6s01Vq1p30n01=3c-tct264QTpV--Q@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

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.

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

> ! 	/*
> ! 	 * 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 && ..."

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

- Heikki


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers