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

On Mon, Jun 17, 2019 at 5:41 PM Stephen Frost <sfrost(at)snowman(dot)net> wrote:

>
> * Ian Barwick (ian(dot)barwick(at)2ndquadrant(dot)com) wrote:
> > On 6/15/19 1:08 AM, Stephen Frost wrote:
> > > * Ian Barwick (ian(dot)barwick(at)2ndquadrant(dot)com) wrote:
>
> > >> Attached patch attempts to rectify this situation by having
> replace_auto_config_value()
> > >> deleting any duplicate entries first, before making any changes to
> the last entry.
> > >
> > > While this might be a good belt-and-suspenders kind of change to
> > > include, I don't think pg_basebackup should be causing us to have
> > > multiple entries in the file in the first place..
> > (...)
> > >> Also attached is a set of TAP tests to check ALTER SYSTEM works as
> expected (or
> > >> at least as seems correct to me).
> > >
> > > In my view, at least, we should have a similar test for pg_basebackup
> to
> > > make sure that it doesn't create an invalid .auto.conf file.
> >
> > Indeed... I'd be happy to create tests... but first we need a definition
> of what
> > constitutes a valid .auto.conf file.
> >
> > If that definition includes requiring that a parameter may occur only
> once, then
> > we need to provide a way for utilities such as pg_basebackup to write
> the replication
> > configuration to a configuration file in such a way that it doesn't
> somehow
> > render that file invalid.
>
> Yes, I think that we do need to require that a parameter only occur once
> and pg_basebackup and friends need to be able to manage that.
>

+1.

> > I agere that there should have been some effort put into making the way
>
> > ALTER SYSTEM is modified be consistent between the backend and utilities
> > > like pg_basebackup (which would also help third party tools understand
> > > how a non-backend application should be modifying the file).
> >
> > Did you mean to say "the way postgresql.auto.conf is modified"?
>
> Ah, yes, more-or-less. I think I was going for 'the way ALTER SYSTEM
> modifies postgresql.auto.conf'.
>
> > I suggest explicitly documenting postgresql.auto.conf behaviour (and the
> circumstances
> > where it's acceptable to modify it outside of ALTER SYSTEM calls) in the
> documentation
> > (and possibly in the code), so anyone writing utilities which need to
> > append to postgresql.auto.conf knows what the situation is.
>
> Yeah, I would think that, ideally, we'd have some code in the common
> library that other utilities could leverage and which the backend would
> also use.
>
> > - postgresql.auto.conf is maintained by PostgreSQL and can be rewritten
> at will by the system
> > at any time
>
> I'd further say something along the lines of 'utilities should not
> modify a postgresql.auto.conf that's in place under a running PostgreSQL
> cluster'.
>

Do we need to differ between "external" and "internal" utilities here?

> > - there is no guarantee that items in postgresql.auto.conf will be in a
> particular order
> > - it must never be manually edited (though it may be viewed)
>
> 'must' is perhaps a bit strong... I would say something like
> "shouldn't", as users may *have* to modify it, if PostgreSQL won't
> start due to some configuration in it.
>

+1.

> - postgresql.auto.conf may be appended to by utilities which need to
> write configuration
> > items and which and need a guarantee that the items will be read by
> the server at startup
> > (but only when the server is down of course)
>
> Well, I wouldn't support saying "append" since that's what got us into
> this mess. :)
>
> > - any duplicate items will be removed when ALTER SYSTEM is executed to
> change or reset
> > an item (a WARNING will be emitted about duplicate items removed)
> > - comment lines (apart from the warning at the top of the file) will be
> silently removed
> > (this is currently the case anyway)
>
> I'd rather say that 'any duplicate items should be removed, and a
> WARNING emitted when detected', or something along those lines. Same
> for comment lines...
>

I think it's perfectly fine to silently drop comments (other than the one
at the very top which says don't touch this file).

//Magnus