Message-ID: <5236B6A2.8080607@vmware.com>
Date: Mon, 16 Sep 2013 10:43:30 +0300
From: Heikki Linnakangas <hlinnakangas@vmware.com>
Organization: VMware
User-Agent: Mozilla/5.0 (X11; Linux x86_64;
 rv:10.0.12) Gecko/20130116 Icedove/10.0.12
MIME-Version: 1.0
To: Alexander Korotkov <aekorotkov@gmail.com>
CC: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: GIN improvements part 1: additional information
References: 
 <CAPpHfduxv-iL7aedwPW0W5fXrWGAKfxijWM63_hZujaCRxnmFQ@mail.gmail.com>
 <CAPpHfduQrtC4B+MC9_z3QB8oJsK1Ws-H8ivEsy_cmDnfoJqkeQ@mail.gmail.com>
 <CAPpHfdszEJhSynTPX6=tB3aKq1EA1JXpwx4xiFYM097K7PhQLA@mail.gmail.com>
 <CAPpHfdu8=5spaS7c9mxwH7bNMDR6n378fZCv6c2Y3YM7iE5KiQ@mail.gmail.com>
 <51CEA13C.8040103@vmware.com>
 <CAPpHfdutJEC9+6vWX579afZKzdHKCT2wzDkAEom1XmiXEwF3pw@mail.gmail.com>
In-Reply-To: 
 <CAPpHfdutJEC9+6vWX579afZKzdHKCT2wzDkAEom1XmiXEwF3pw@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 15.09.2013 12:14, Alexander Korotkov wrote:
> On Sat, Jun 29, 2013 at 12:56 PM, Heikki Linnakangas<
> hlinnakangas@vmware.com>  wrote:
>
>> There's a few open questions:
>>
>> 1. How are we going to handle pg_upgrade? It would be nice to be able to
>> read the old page format, or convert on-the-fly. OTOH, if it gets too
>> complicated, might not be worth it. The indexes are much smaller with the
>> patch, so anyone using GIN probably wants to rebuild them anyway, sooner or
>> later. Still, I'd like to give it a shot.
>>
>> 2. The patch introduces a small fixed 32-entry index into the packed
>> items. Is that an optimal number?
>>
>> 3. I'd like to see some performance testing of insertions, deletions, and
>> vacuum. I suspect that maintaining the 32-entry index might be fairly
>> expensive, as it's rewritten on every update to a leaf page.
>
> It appears that maintaining 32-entry index is really expensive because it
> required re-decoding whole page. This issue is fixed in attached version of
> patch by introducing incremental updating of that index. Benchmarks will be
> posted later today..

Great! Please also benchmark WAL replay; you're still doing 
non-incremental update of the 32-entry index in ginRedoInsert.

A couple of quick comments after a quick glance (in addition to the above):

The varbyte encoding stuff is a quite isolated piece of functionality. 
And potentially useful elsewhere, too. It would be good to separate that 
into a separate .c/.h files. I'm thinking of 
src/backend/access/gin/packeditemptr.c, which would contain 
ginDataPageLeafReadItemPointer, ginDataPageLeafWriteItemPointer and 
ginDataPageLeafGetItemPointerSize functions. A new typedef for 
varbyte-encoded things would probably be good too, something like 
"typedef char *PackedItemPointer". In the new .c file, please also add a 
file-header comment explaining the encoding.

The README really needs to be updated. The posting tree page structure 
is a lot more complicated now, there needs to be a whole new section to 
explain it. A picture would be nice, similar to the one in bufpage.h.

It's a bit funny that we've clumped together all different kinds of GIN 
insertions into one WAL record type. ginRedoInsert does completely 
different things depending on what kind of a page it is. And the 
ginXlogInsert struct also contains different kinds of things depending 
on what kind of an insertion it is. It would be cleaner to split it into 
three. (this isn't your patch's fault - it was like that before, too.)

- Heikki


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