Date: Mon, 5 Aug 2019 16:55:20 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Stephen Frost <sfrost@snowman.net>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Amit Kapila <amit.kapila16@gmail.com>,
	Ian Barwick <ian.barwick@2ndquadrant.com>,
	Robert Haas <robertmhaas@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
Message-ID: <20190805145520.aezlej5gjxohfclq@development>
References: <20190624185647.GW2480@tamriel.snowman.net>
 <15032.1561403529@sss.pgh.pa.us>
 <20190624192014.GB2480@tamriel.snowman.net>
 <56aff4f3-dec8-f462-7feb-0938411b04a6@2ndquadrant.com>
 <CAA4eK1+JMm=tvx720y6jhD8pdzt=HdS1-TwQwkARib2pwK8EvQ@mail.gmail.com>
 <20190802222254.qsvqjtkwte464lu3@development>
 <31697.1564784856@sss.pgh.pa.us>
 <CAOuzzgqNyQM_z0biEEBVBK9WcaLmbCyWrd5xTdf-Oxb_9pntLA@mail.gmail.com>
 <20190802230011.dqokdhjptykopxxc@development>
 <20190805142139.GW29202@tamriel.snowman.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190805142139.GW29202@tamriel.snowman.net>
User-Agent: NeoMutt/20180716-1444-295967
Precedence: bulk

On Mon, Aug 05, 2019 at 10:21:39AM -0400, Stephen Frost wrote:
>Greetings,
>
>* Tomas Vondra (tomas.vondra@2ndquadrant.com) wrote:
>> On Fri, Aug 02, 2019 at 06:38:46PM -0400, Stephen Frost wrote:
>> >On Fri, Aug 2, 2019 at 18:27 Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> >>Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>> >>> There seems to be a consensus that this this not a pg_basebackup issue
>> >>> (i.e. duplicate values don't make the file invalid), and it should be
>> >>> handled in ALTER SYSTEM.
>> >>
>> >>Yeah.  I doubt pg_basebackup is the only actor that can create such
>> >>situations.
>> >>
>> >>> The proposal seems to be to run through the .auto.conf file, remove any
>> >>> duplicates, and append the new entry at the end. That seems reasonable.
>> >>
>> >>+1
>> >
>> >I disagree that this should only be addressed in alter system, as I’ve said
>> >before and as others have agreed with.  Having one set of code that can be
>> >used to update parameters in the auto.conf and then have that be used by
>> >pg_basebackup, alter system, and external tools, is the right approach.
>> >
>> >The idea that alter system should be the only thing that doesn’t just
>> >append changes to the file is just going to lead to confusion and bugs down
>> >the road.
>>
>> I don't remember any suggestions ALTER SYSTEM should be the only thing
>> that can rewrite the config file, but maybe it's buried somewhere in the
>> thread history. The current proposal certainly does not prohibit any
>> external tool from doing so, it just says we should expect duplicates.
>
>There's an ongoing assumption that's been made that only ALTER SYSTEM
>could make these changes because nothing else has the full GUC system
>and a running PG instance to validate everything.
>
>The suggestion that an external tool could do it goes against that.
>
>If we can't, for whatever reason, work our way towards having code that
>external tools could leverage to manage .auto.conf, then if we could at
>least document what the expectations are and what tools can/can't do
>with the file, that would put us in a better position than where we are
>now.
>

IMO documenting the basic rules, and then doing some cleanup/validation
at instance start is the only practical solution, really.

You can't really validate "everything" without a running instance,
because that's the only place where you have GUCs defined by extensions.
I don't see how that could work for external tools, expected to run
exactly when the instance is not running.

I can't think of a use case where simply appending to the file would not
be perfectly sufficient. You can't really do much when the instance is
not running.


>I strongly believe that whatever the rules and expectations are that we
>come up with, both ALTER SYSTEM and the in-core and external tools
>should follow them.
>

I'm not against giving external tools such capability, in whatever way
we think is appropriate (library, command-line binary, ...).

I'm against (a) making that a requirement for the external tools,
instead of just allowing them to append to the file, and (b) trying to
do that in PG12. We're at beta3, we don't even have any patch, and it
does quite work for past releases (although it's not that pressing
there, thanks to still having recovery.conf).

>If we say to that tools should expect duplicates in the file, then
>ALTER SYSTEM should as well, which was the whole issue in the first
>place- ALTER SYSTEM didn't expect duplicates, but the external tools and
>the GUC system did.
>

Sure.

>If we say that it's acceptable for something to remove duplicate GUC
>entries from the file, keeping the last one, then external tools should
>feel comfortable doing that too and we should make it clear what
>"duplicate" means here and how to identify one.
>

Sure. I don't see why the external tools would bother with doing that,
but I agree there's no reason not to document what duplicates mean.

>If we say it's optional for a tool to remove duplicates, then we should
>point out the risk of "running out of disk space" for tool authors to
>consider.  I don't agree with the idea that tool authors should be asked
>to depend on someone running ALTER SYSTEM to address that risk.  If
>there's a strong feeling that tool authors should be able to depend on
>PG to perform that cleanup for them, then we should use a mechanism to
>do so which doesn't involve an entirely optional feature.
>

Considering the external tools are only allowed to modify the file while
the instance is not running, and that most instances are running all the
time, I very much doubt this is a risk we need to worry about.

And I don't see why we'd have to run ALTER SYSTEM - I proposed to do the
cleanup at instance start too.

>For reference, all of the above, while not as cleanly as it could have
>been, was addressed with the recovery.conf/recovery.done system.  Tool
>authors had a good sense that they could replace that file, and that PG
>would clean it up at exactly the right moment, and there wasn't this
>ugly interaction with ALTER SYSTEM to have to worry about.  That none of
>this was really even discussed or addressed previously even after being
>pointed out is really disappointing.
>
>Just to be clear, I brought up this exact concern back in *November*:
>
>https://www.postgresql.org/message-id/20181127153405.GX3415%40tamriel.snowman.net
>
>And now because this was pushed forward and the concerns that I raised
>ignored, we're having to deal with this towards the end of the release
>cycle instead of during normal development.  The things we're talking
>about now and which I'm getting push-back on because of the release
>cycle situation were specifically suggestions I made in the above email
>in November where I also brought up concern that ALTER SYSTEM would be
>confused by the duplicates- giving external tools guideance on how to
>modify .auto.conf, or providing them a tool (or library), or both.
>
>None of this should be coming as a surprise to anyone who was following
>and I feel we should be upset that this was left to such a late point in
>the release cycle to address these issues.
>

I have not been following that discussion, but I acknowledge you've
raised those points before. At this point I'm really interested in this
as a RMT member, and from that position I don't quite care what happened
in November - my concern is what to do now, so that we can get 12 out.


>> >>There was a discussion whether to print warnings about the duplicates. I
>> >>> personally see not much point in doing that - if we consider duplicates
>> >>> to be expected, and if ALTER SYSTEM has the license to rework the config
>> >>> file any way it wants, why warn about it?
>> >>
>> >>Personally I agree that warnings are unnecessary.
>> >
>> >And at least Magnus and I disagree with that, as I recall from this
>> >thread.  Let’s have a clean and clear way to modify the auto.conf and have
>> >everything that touches the file update it in a consistent way.
>>
>> Well, I personally don't feel very strongly about it. I think the
>> warnings will be a nuisance bothering people with expeced stuff, but I'm
>> not willing to fight against it.
>
>I'd be happier with one set of code at least being the recommended
>approach to modifying the file and only one set of code in our codebase
>that's messing with .auto.conf, so that, hopefully, it's done
>consistently and properly and in a way that everything agrees on and
>expects, but if we can't get there due to concerns about where we are in
>the release cycle, et al, then let's at least document what is
>*supposed* to happen and have our code do so.
>

I think fixing ALTER SYSTEM to handle duplicities, and documenting the
basic rules about modifying .auto.conf is the way to go.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services