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
> - 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
> - 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.
> ITAGAKI Takahiro
> NTT Open Source Software Center
In response to
pgsql-hackers by date
|Next:||From: Peter Eisentraut||Date: 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 Eisentraut||Date: 2009-01-20 05:44:59|
|Subject: Re: foreign_data test fails with non-C locale|