Skip site navigation (1) Skip section navigation (2)

Re: V4 of PITR performance improvement for 8.4

From: Koichi Suzuki <koichi(dot)szk(at)gmail(dot)com>
To: ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: V4 of PITR performance improvement for 8.4
Date: 2009-01-20 06:21:42
Message-ID: a778a7260901192221n4a043d05m32fc7c6eecbb76c8@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
Thanks a lot for the review.

Enclosed is a revised patch reflecting your comments.   Also, some
reply to the comments are included inline.

2009/1/14 ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>:
>
> "Koichi Suzuki" <koichi(dot)szk(at)gmail(dot)com> wrote:
>
>> > This patc also include additional patch for posix_fadvise to skip
>> > prefetch of pages which does not exist.
>
> I reviewed your patch and found items to be fixed and improved.
>
> - You should not claim your copyrights.
> There are your copyright claims in two files newly added, but they
> should be included by PostgreSQL Global Development Group.
> | * Portions Copyright (c) 2008, Nippon Telegraph and Telephone Corporation
> | * Portions Copyright (c) 1996-2004, PostgreSQL Global Development Group

Okay, fixed.

>
> - Avoid pending wal records on continuous REDO.
> In archive recovery, ReadRecord() waits until the next segment comes.
> Should we avoid pending WAL records during the waiting?
> RedoRecords() might be needed before calling RestoreArchivedFile().
> It would be better to redo records and restore archived files
> in parallel, but we will need to replace system() to other system
> because system() blocks until the process finishes.

Yes, I added this feature.   Because system() blocks, it does not run
in parallel.

>
> - Should check the buffer pool before calling smgrprefetch.
> If the page is in the shared buffer pool, we don't need to
> prefetch the buffer from disks. I think it is good to add
> PrefetchBufferWithoutRelcache() like ReadBufferWithoutRelcache().

Agreed.   This needs an addition to buffer manager API.  I'd like to
submit this change as a separate patch by the end of this week.

>
> - Is possible to remove calls of ReadAheadHasRoom()?
> If we checks rooms of readahead-queue, xxx_readahead functions
> don't need to call ReadAheadHasRoom. The maximum readaheads are
> at most 3, so we'll flush the queue when the rooms of the queue
> are less than 3. And if the queue is full unexpectedly, we can
> ignore the added items because it is only hint.

More than three pages are involved in GiST WAL record so I think the
above is needed.

>
> - Should avoid large static variables.
> | [src/backend/access/transam/xlog]
> | + static char RecordQueueBuf[XLogSegSize * 2];
> | [readahead.c]
> | + static XLogReadAhead ReadAheadQueue[ReadAheadQueueSize];
> I think it is a bad idea to place a large object as a static variable.
> It should be a pointer and allocated with palloc() in runtime.
> Also those variable are only used by startup process.

In startup process, most of memory allocation is done by malloc(), not
palloc().  Replacing malloc() by palloc() may be done separately.

>
> - Merge readahead.h to xlog.h or something.
> It export just a few functions and the functionality belongs
> xlog and recovery.

Agreed.

>
> Regards,
> ---
> ITAGAKI Takahiro
> NTT Open Source Software Center
>
>
>



-- 
------
Koichi Suzuki

Attachment: readahead-20090116.patch
Description: text/x-diff (40.0 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Peter EisentrautDate: 2009-01-20 07:25:44
Subject: Re: tsearch with Turkish locale ( was Re: foreign_data test fails with non-C locale)
Previous:From: Peter EisentrautDate: 2009-01-20 05:44:59
Subject: Re: foreign_data test fails with non-C locale

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group