Re: Renaming of pg_xlog and pg_clog

On Fri, Sep 30, 2016 at 1:45 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)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