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

Hi

On 6/15/19 1:08 AM, Stephen Frost wrote:
 > Greetings,
 >
 > * Ian Barwick (ian.barwick@2ndquadrant.com) wrote:
 >> Consider the following cascading standby setup with PostgreSQL 12:
 >>
 >> - there exists a running primary "A"
 >> - standby "B" is cloned from primary "A" using "pg_basebackup --write-recovery-conf"
 >> - standby "C" is cloned from standby "B" using "pg_basebackup --write-recovery-conf"
(...)
 >> However, executing "ALTER SYSTEM SET primary_conninfo = 'host=someothernode'" left
 >> standby "C"'s "postgresql.auto.conf" file looking like this:
 >>
 >>     # Do not edit this file manually!
 >>     # It will be overwritten by the ALTER SYSTEM command.
 >>     primary_conninfo = 'host=someothernode'
 >>     primary_conninfo = 'host=node_B'
 >>
 >> which seems somewhat broken, to say the least.
 >
 > Yes, it's completely broken, which I've complained about at least twice
 > on this list to no avail.
 >
 > Thanks for putting together an example case pointing out why it's a
 > serious issue.  The right thing to do here it so create an open item for
 > PG12 around this.

Done.

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

In Pg11 and earlier, it was just a case of writing (or overwriting) recovery.conf.

In Pg12, the code in pg_basebackup implies the correct thing to do is append to .auto.conf,
but as demonstrated that can cause problems with duplicate entries.

Having pg_basebackup, or any other utility which clones a standby, parse the file
itself to remove duplicates seems like a recipe for pain and badly duplicated effort
(unless there's some way of calling the configuration parsing code while the
server is not running).

We could declare that the .auto.conf file will be reset to the default state when
a standby is cloned, but the implicit behaviour so far has been to copy the file
as-is (as would happen with any other configuration files in the data directory).

We could avoid the need for modifying the .auto.conf file by declaring that a
configuration with a specific name in the data directory (let's call it
"recovery.conf" or "replication.conf") can be used by any utilities writing
replication configuration (though of course in contrast to the old recovery.conf
it would be included, if exists, as a normal configuration file, though then the
precedence would need to be defined, etc..). I'm not sure off the top of my head
whether something like that has already been discussed, though it's presumably a
bit late in the release cycle to make such changes anyway?

 >>> This is absolutely the fault of the system for putting in multiple
 >>> entries into the auto.conf, which it wasn't ever written to handle.
 >>
 > * Amit Kapila (amit.kapila16@gmail.com) wrote:
 >> Right.  I think if possible, it should use existing infrastructure to
 >> write to postgresql.auto.conf rather than inventing a new way to
 >> change it.  Apart from this issue, if we support multiple ways to edit
 >> postgresql.auto.conf, we might end up with more problems like this in
 >> the future where one system is not aware of the way file being edited
 >> by another system.
 >
 > 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"?

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.

Something along the following lines?

- postgresql.auto.conf is maintained by PostgreSQL and can be rewritten at will by the system
   at any time
- 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)
- 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)
- 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 will happily work on those changes in the next few days if agreed.



Regards

Ian Barwick

-- 
  Ian Barwick                   https://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services