Date: Tue, 10 Sep 2019 14:52:19 -0300
From: Alvaro Herrera from 2ndQuadrant <alvherre@alvh.no-ip.org>
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: <20190910175219.GA8826@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAEET0ZHXKepP5ebMtqgqmYeCpzoLGGxzA528yrcP-WdmOSQt4A@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On 2019-Sep-09, Paul Guo wrote:

> >
> > 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.
> 
> How about renaming the function names to
> GenerateRecoveryConf -> GenerateRecoveryConfContents
> WriteRecoveryConf -> WriteRecoveryConfInfo    <- it writes standby.signal
> on pg12+, so function name WriteRecoveryConfContents is not accurate.

GenerateRecoveryConfig / WriteRecoveryConfig ?

> > 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.
> >
> I thought some about where to put the common code also. It seems pg_rewind
> and pg_basebackup are the only consumers of the small common code.  I doubt
> it deserves a separate file under src/fe_utils.

Hmm, but other things there are also used by only two programs, say
psqlscan.l and conditional.c are just for psql and pgbench.

> > 0003:
> >
> > I still don't understand why we need a command-line option to do this.
> > Why should it not be the default behavior?
> 
> This was discussed but frankly speaking I do not know how other postgres
> users or enterprise providers handle this (probably some have own scripts?).
> I could easily remove the option code if more and more people agree on that
> or at least we could turn it on by default?

Well, I've seen no contrary votes, and frankly I see no use for the
opposite (current) behavior.

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