Re: Renaming of pg_xlog and pg_clog - Mailing list pgsql-hackers
From | Michael Paquier |
---|---|
Subject | Re: Renaming of pg_xlog and pg_clog |
Date | |
Msg-id | CAB7nPqRD0=nuZOgODOEcPsGxj9+C_RAUyNFF+4UdUesCbif2tg@mail.gmail.com Whole thread Raw |
In response to | Re: Renaming of pg_xlog and pg_clog (Robert Haas <robertmhaas@gmail.com>) |
Responses |
Re: Renaming of pg_xlog and pg_clog
(David Steele <david@pgmasters.net>)
Re: Renaming of pg_xlog and pg_clog (Robert Haas <robertmhaas@gmail.com>) Re: Renaming of pg_xlog and pg_clog (Peter Eisentraut <peter.eisentraut@2ndquadrant.com>) |
List | pgsql-hackers |
On Mon, Oct 3, 2016 at 10:00 PM, Robert Haas <robertmhaas@gmail.com> wrote: > 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 */ 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@mail.gmail.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
Attachment
pgsql-hackers by date: