Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions

Here's a radical suggestion: replace postgresql.auto.conf with a directory
containing multiple files. Each file is named after a configuration
parameter, and its content is the value of the parameter.

So to remove a special configuration parameter, delete its file. To set it,
write the file, replacing an existing file if it exists.

For this to work unambiguously we would have to specify an exact,
case-sensitive, form of every parameter name that must be used within the
auto conf directory. I would suggest using the form listed in the
documentation (i.e., lower case, to my knowledge).

In order to prevent confusing and surprising behaviour, the system should
complain vociferously if it finds a configuration parameter file that is
not named after a defined parameter, rather than just ignoring it.

On Mon, 5 Aug 2019 at 10:21, Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> Greetings,
>
> * Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > >>Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)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.
>
> 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.
>
> 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.
>
> 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.
>
> 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.
>
> 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.
>
> > >>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.
>
> Thanks,
>
> Stephen
>