Re: V4 of PITR performance improvement for 8.4

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

- 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().

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

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

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

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