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

On Fri, Jun 14, 2019 at 9:38 PM Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Ian Barwick (ian(dot)barwick(at)2ndquadrant(dot)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 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.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com