Re: Renaming of pg_xlog and pg_clog

On Mon, Oct 3, 2016 at 10:00 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> 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 */

I have adopted the existing style of BaseBackup() in last versions,
but I don't mind as well doing the way you suggest, without changing
the stuff in BaseBackup() though, we'd still want a pg_basebackup
compiled with 10.1 to work with 10.2 or newer minor versions of 10.

> 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.

Fixed. That was done without brackets on HEAD, but you are right that
is not PG-like. (There are other places in src/bin/pg_basebackup doing
the same thing, but I have not modified them to not create more diff
noise).

> 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.

strerror(errno) needs to be used instead of getErrorText() in the
context of this patch.

> 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.

Yes, pg_wal is fine as new name in replacement of pg_xlog. Now for the
pg_clog...

Re: Michael Paquier 2016-09-30
<CAB7nPqR=SZNo_=B1ukwCiNn7aWDcw_dV0z-LG4ys9WF1N4a=uQ(at)mail(dot)gmail(dot)com>
> "pg_trans" is used in two places:
> -pg_clog records the commit status for each transaction that has been assigned
> +pg_trans records the commit status for each transaction that has been assigned

> - /* copy old commit logs to new data dir */
> - copy_subdir_files("pg_clog");
> + /*
> + * Copy old commit logs to new data dir. pg_clog has been renamed to
> + * pg_trans in post-10 clusters.

> (Fwiw, I'd prefer something shorter than the imho clumsy
> pg_transaction. "pg_xact" sounded very fine for me, it also combines
> nicely with pg_subxact and the existing pg_multixact.)

Thanks, fixed those.

So this is still open for votes. Here are the candidates and who
voiced for what:
- pg_transaction: Michael P, Thomas M. => Current 0002 is doing that.
- pg_xact: David S, Robert
- pg_trans: Tom
- pg_transaction_status: Peter E.

Attached is a new patch set. I have re-done tests with pg_upgrade with
9.5 -> HEAD and 9.6 -> HEAD, and pg_basebackup for symlink handling.
Actually in the latter case I have found a bug in 0001 introduced by
bc34223b because fsync_pgdata() is not aware of the version of the
server it is trying to fsync(). I think that the better way to address
that is to extend fsync_pgdata in file_utils.c with the server version
to fsync() like attached.
--
Michael