Re: WAL replay bugs

OK, I have been working more on this patch, improving on-the-fly the
following things on top of what Horiguchi-san has reported:
- Moved sequence page opaque data to sequence.h, renaming it at the same time.
- Improvement of page type identification, particularly for sequences
using a correct opaque data structure. For gin the process is not that
cool, but I guess that there is nothing much to do as it has no proper
page identifier :(

On Thu, Jul 3, 2014 at 7:34 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> ===== 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.
Yes, it is worth mentioning and a comment in bufcapt.h seems enough.

> - 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.
Yesh, this seems informative.

> - buffer_capture_is_changed()
>
>  The name for the function seems 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 clearer.
Hm, yes. This name looks better fine as it remains static within bufcapt.c.

> ====== bufmgr.c:
>
> - ConditionalLockBuffer()
>
>  Sorry for a trivial comment, but the variable 'res' conceales
>  the meaning. "acquired" seems to be more preferable, isn't it?
Fixed.

> - 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 think that there is much to gain with such macros as of now
LWLock->exclusive is only used in the code this patch introduces.

> # 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.
Well yes.

> ===== bufpage.c:
>
> -  #include "storage/bufcapt.h"
>
>   The include seems to be needless.
Yep.

> ===== bufcapt.h:
>
> - header comment
>
>   The file name is misspelled as 'bufcaptr.h'.
Nicely spotted.

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

> ===== 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 seems 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.
Hm, no. As of now, there is still some margin:
PG_VERSION_NUM = 90500
PG_VERSION_NUM % 16384 + 49152 = 57732

> - 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..
Hm. You could complain about pg_upgrade as well now for the same
thing. But I guess that it doesn't hurt to complain back to caller
about the range of ports already in use, so changed this way.

Regards,
--
Michael