On Fri, Sep 30, 2016 at 1:45 AM, Michael Paquier <michael.paqu...@gmail.com> wrote: > As there have been no reviews at code level, I am moving that to the next CF.
Code review of 0001: I think the tests for PQserverVersion(conn) / 100 >= 1000 are strange. I submit that either PQserverVersion(conn) >= 100000 or PQserverVersion(conn) / 10000 >= 10 is an easier-to-understand test. I vote for the first style. So in pg_basebackup.c I'd do: #define MINIMUM_VERSION_FOR_PG_WAL 100000 ... int version = PQserverVersion(conn); ... if (version >= MINIMUM_VERSION_FOR_PG_WAL) /* do whatever */ Also, for this sort of thing: + if (!conn) + /* Error message already written in GetConnection() */ + exit(1); ...because of the comment, I'd add braces around this. Tom changed the error-reporting framework for pg_upgrade in f002ed2b8e45286fe73a36e119cba2016ea0de19, so you'll need to do some rebasing over that. I haven't checked what the new conventions are, but obviously you'll want to try to be consistent with them. Other than those minor nitpicks, I think this looks OK. I'm not sure we have consensus on 0002, but 0001 seems to be popular with far more people than not. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
