From: Stephen Frost <sfrost@snowman.net>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Ian Barwick <ian.barwick@2ndquadrant.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
Date: Sun, 16 Jun 2019 13:16:11 -0400
Message-ID: <20190616171610.GO2480@tamriel.snowman.net> (raw)
In-Reply-To: <CAA4eK1JW9UfLOtMEJq5b_YcogbWdxzAGUzNi72F2Xx4r1_h6QA@mail.gmail.com>
References: <aed6cc9f-98f3-2693-ac81-52bb0052307e@2ndquadrant.com>
<20190614160835.GF2480@tamriel.snowman.net>
<CAA4eK1JW9UfLOtMEJq5b_YcogbWdxzAGUzNi72F2Xx4r1_h6QA@mail.gmail.com>
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
Attachments:
signature.asc (application/pgp-signature, 818B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: sfrost@snowman.net, amit.kapila16@gmail.com, ian.barwick@2ndquadrant.com
Subject: Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
In-Reply-To: <20190616171610.GO2480@tamriel.snowman.net>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox