Re: WAL replay bugs - Mailing list pgsql-hackers

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.paquier@gmail.com> wrote in
<CAB7nPqT_fs_3jLNHYWC6nzej4sBL6DGsLFVCg0JBUkgjeP9Tfw@mail.gmail.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.kyotaro@lab.ntt.co.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 youthought 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