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.paqu...@gmail.com> wrote in <cab7npqt_fs_3jlnhywc6nzej4sbl6dgslfvcg0jbukgjep9...@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.kyot...@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 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
