Date: Thu, 5 Sep 2019 09:23:11 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Paul Guo <pguo@pivotal.io>
Cc: Michael Paquier <michael@paquier.xyz>,
	Thomas Munro <thomas.munro@gmail.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>,
	Jimmy Yih <jyih@pivotal.io>, Ashwin Agrawal <aagrawal@pivotal.io>
Subject: Re: Two pg_rewind patches (auto generate recovery conf and ensure
 clean shutdown)
Message-ID: <20190905132311.GA22775@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAEET0ZG5mCxUYVkZTeh3S+f7Qsp+QAt5APdyPagYhtZf-a8MXw@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

Thank for rebasing.

I didn't like 0001 very much.

* It seems now would be the time to stop pretending we're using a file
called recovery.conf; I know we still support older server versions that
use that file, but it sounds like we should take the opportunity to
rename the function to be less misleading once those versions vanish out
of existance.

* disconnect_and_exit seems a bit out of place compared to the other
parts of this new module.  I think you only put it there so that the
'conn' can be a global, and that you can stop passing 'conn' as a
variable to GenerateRecoveryConf.  It seems more modular to me to keep
it as a separate variable in each program and have it passed down to the
routine that writes the file.

* From modularity also seems better to me to avoid a global variable
'recoveryconfcontents' and instead return the string from
GenerateRecoveryConf to pass as a param to WriteRecoveryConf.
(In fact, I wonder why the current structure is like it is, namely to
have ReceiveAndUnpackTarFile write the file; why wouldn't be its caller
be responsible for writing it?)

I wonder about putting this new file in src/fe_utils instead of keeping
it in pg_basebackup and symlinking to pg_rewind.  Maybe if we make it a
true module (recovery_config_gen.c) it makes more sense there.

0002 seems okay as far as it goes.


0003:

I still don't understand why we need a command-line option to do this.
Why should it not be the default behavior?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services