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

* Ian Barwick (ian.barwick@2ndquadrant.com) wrote:
> On 6/15/19 1:08 AM, Stephen Frost wrote:
> > * Ian Barwick (ian.barwick@2ndquadrant.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.

> > 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'.

> - 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.

> - 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...