On Fri, Sep 30, 2016 at 1:45 AM, Michael Paquier
<michael.paquier@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