Re: WAL replay bugs

Hello, thank you for considering my comments, including somewhat
impractical ones.

I'll have a look at the latest patch sooner.

At Fri, 4 Jul 2014 15:29:51 +0900, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote in <CAB7nPqT_fs_3jLNHYWC6nzej4sBL6DGsLFVCg0JBUkgjeP9Tfw(at)mail(dot)gmail(dot)com>
> 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 :(

Year, there seems to be no choice than that.

> On Thu, Jul 3, 2014 at 7:34 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > ===== bufcapt.c:
> >
> > - buffer_capture_remember() or so.
...
> Yes, it is worth mentioning and a comment in bufcapt.h seems enough.
>
> > - buffer_capture_forget()
...
> Yesh, this seems informative.
>
> > - buffer_capture_is_changed()
...
> Hm, yes. This name looks better fine as it remains static within bufcapt.c.
>
> > ====== bufmgr.c:
> >
> > - ConditionalLockBuffer()
...
> Fixed.
>
> > - LockBuffer()
...
> > 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.

Year, I think so, too:p That's simply for the case if you
thought that.

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

That's great.

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

Thank you ;)

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

Yes, it's practically no problem. I said about the theroretical
max value seeing it without any preassumption about the value of
PG_VERSION_NUM. There's in reality no problem before the
PostgreSQL 9.82.88 comes, and 10.0.0 doesn't cause problem. So
I'm not so dissapointed if it is not fixed.

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

Yes, this comment is also comes from a kind of
fastidiousness. I'm satisified not to fixed if you think so.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center