Re: WAL replay bugs

Hello, This is the additional comments for other part.

I haven't see significant defect in the code so far.

===== bufcapt.c:

- buffer_capture_remember() or so.

Pages for unlogged tables are avoided to be written taking
advantage that the lsn for such pages stays 0/0. I'd like to see
a comment mentioning for, say, buffer_capture_is_changed? or
buffer_capture_forget or somewhere.

- buffer_capture_forget()

However this error is likely not to occur, in the error message
"could not find image...", the buffer id seems to bring no
information. LSN, or quadraplet of LSN, rnode, forknum and
blockno seems to be more informative.

- buffer_capture_is_changed()

The name for the function semes to be misleading. What this
function does is comparing LSNs between stored page image and
current page. lsn_is_changed(BufferImage) or something like
would be more clearly.

====== bufmgr.c:

- ConditionalLockBuffer()

Sorry for a trivial comment, but the variable 'res' conceales
the meaning. "acquired" seems to be more preferable, isn't it?

- LockBuffer()

There is a 'XXX' comment. The discussion written there seems to
be right, for me. If you mind that peeking into there is a bad
behavior, adding a macro into lwlock.h would help:p

lwlock.h: #define LWLockHoldedExclusive(l) ((l)->exclusive > 0)
lwlock.h: #define LWLockHoldedShared(l) ((l)->shared > 0)

# I don't see this usable so much..

bufmgr.c: if (LWLockHoldedExclusive(buf->content_lock))

If there isn't any particular concern, 'XXX:' should be removed.

===== bufpage.c:

- #include "storage/bufcapt.h"

The include seems to be needless.

===== bufcapt.h:

- header comment

The file name is misspelled as 'bufcaptr.h'.
Copyright notice of UC is needed? (Other files are the same)

- The includes in this header except for buf.h seem not to be
necessary.

===== init_env.sh:

- pg_get_test_port()

It determines server port using PG_VERSION_NUM, but is it
necessary? Addition to it, the theoretical maximum initial
PGPORT semes to be 65535 but this script search for free port
up to the number larger by 16 from the start, which would be
beyond the edge.

- pg_get_test_port()

It stops with error after 16 times of failure, but the message
complains only about the final attempt. If you want to mention
the port numbers, it might should be 'port $PGPORTSTART-$PGPORT
..' or would be 'All 16 ports attempted failed' or something..

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center