Date: Sun, 16 Jun 2019 13:16:11 -0400
From: Stephen Frost <sfrost@snowman.net>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Ian Barwick <ian.barwick@2ndquadrant.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
Message-ID: <20190616171610.GO2480@tamriel.snowman.net>
References: <aed6cc9f-98f3-2693-ac81-52bb0052307e@2ndquadrant.com>
 <20190614160835.GF2480@tamriel.snowman.net>
 <CAA4eK1JW9UfLOtMEJq5b_YcogbWdxzAGUzNi72F2Xx4r1_h6QA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="W9xFwOr4e03bkzx5"
Content-Disposition: inline
In-Reply-To: 
 <CAA4eK1JW9UfLOtMEJq5b_YcogbWdxzAGUzNi72F2Xx4r1_h6QA@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--W9xFwOr4e03bkzx5
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 (o=
r any other settings
> > > formerly in recovery.conf), it has just manifested itself as the buil=
t-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 configurati=
on to
> > > "postgresql.auto.conf" in lieu of creating "recovery.conf" will be pr=
one 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.
>=20
> 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 t=
o be sure to change
> > > the last occurrence in the normal configuration files, however this a=
ctually 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_a=
uto_config_value()
> > > deleting any duplicate entries first, before making any changes to th=
e last entry.
> >
> > While this might be a good belt-and-suspenders kind of change to
> > include,
>=20
> 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

--W9xFwOr4e03bkzx5
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJdBnlaAAoJEO1sijiDR2RVUDYQAJ4Oyd8wIrscA+6wu2Hw7pEh
nvS+ZNL19fvu5Hdw9YmZyVukKfE2eusC4LItZ3UaZ11fyhTV1pTXFc+c0wSh55tH
9MnutovcCMmfFSN0hy2u+V4Z1OGvfdX9ot6EMwYJbAbNOSKgxnF3jGusnY05FjyU
e+0T6mfYyKJjOyLkHbnAMMKtM7aB/nU/4Sqn6yK2bhXjVcA3n2M9f8EMf/q8BPbb
dHRpev1CzAWXIqP1yWOXym2g/WKi2kIhMGBS72tfWeR78ERUaTkvEpkS4rmpxa4M
VuAw8IeOIHdVsuXJfhswyFTWNSYUYG3Zc6c2T3igWBqaJ3SHArpK3dFUNU2o+R5r
kbjT1CNDchgXqfo9tmLt976fsxpQ95Dl1NIsPgPeAPxctac4nYdZYbZlVvF6fSBX
KaPgyvI+4HI5/t84AEI3rTT4fHYr5/I1M+HqRJAm66xAUi1H/LLNWaLzUaewUFm8
t2ZaSpUy6K1SL+oeXVqQFD1M8tLqHtr13Revgezgndx8JY5MKXVumhBHGt47nOZw
TByb3EtAgCCxg/nKuYHfLLsJIvPC+QW00M2jCGAuKiJ9oK1Oz8DhDuy5C/2MGJ8z
6O0RASh835mZjbz+jJPz1qqcoViuhJrVMfu7hmyv/zcg/s6UKA8Qul8e1yS1jW7h
wknrRv+L4FaMRMkor1L2
=tIc4
-----END PGP SIGNATURE-----

--W9xFwOr4e03bkzx5--