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

Greetings,

* Amit Kapila (amit.kapila16@gmail.com) wrote:
> On Fri, Jun 14, 2019 at 9:38 PM Stephen Frost <sfrost@snowman.net> wrote:
> > * Ian Barwick (ian.barwick@2ndquadrant.com) wrote:
> > >
> > > Note this issue is not specific to pg_basebackup, primary_conninfo (or any other settings
> > > formerly in recovery.conf), it has just manifested itself as the built-in toolset as of now
> > > provides a handy way of getting into this situation without too much effort (and any
> > > utilities which clone standbys and append the replication configuration to
> > > "postgresql.auto.conf" in lieu of creating "recovery.conf" will be prone to running into
> > > the same situation).
> >
> > 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.
>
> 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).

> > > I had previously always assumed that ALTER SYSTEM  would change the *last* occurrence for
> > > the parameter in "postgresql.auto.conf", in the same way you'd need to be sure to change
> > > the last occurrence in the normal configuration files, however this actually not the case -
> > > as per replace_auto_config_value() ( src/backend/utils/misc/guc.c ):
> > >
> > >     /* Search the list for an existing match (we assume there's only one) */
> > >
> > > the *first* match is replaced.
> > >
> > > 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,
>
> Another possibility to do something on these lines is to extend Alter
> System Reset <config_param> to remove all the duplicate entries.  Then
> the user has a way to remove all duplicate entries if any and set the
> new value.  I think handling duplicate entries in *.auto.conf files is
> an enhancement of the existing system and there could be multiple
> things we can do there, so we shouldn't try to do that as a bug-fix.

Unless there's actually a use-case for duplicate entries in
postgresql.auto.conf, what we should do is clean them up (and possibly
throw a WARNING or similar at the user saying "something modified your
postgresql.auto.conf in an unexpected way").  I'd suggest we do that on
every ALTER SYSTEM call.

Thanks,

Stephen