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
