On Mon, Jun 17, 2019 at 5:41 PM Stephen Frost <sfr...@snowman.net> wrote:
> > * Ian Barwick (ian.barw...@2ndquadrant.com) wrote: > > On 6/15/19 1:08 AM, Stephen Frost wrote: > > > * Ian Barwick (ian.barw...@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. > +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
