Date: Fri, 14 Jun 2019 12:08:35 -0400
From: Stephen Frost <sfrost@snowman.net>
To: 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
Message-ID: <20190614160835.GF2480@tamriel.snowman.net>
References: <aed6cc9f-98f3-2693-ac81-52bb0052307e@2ndquadrant.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="L5Ks5Ey27iMz7PUy"
Content-Disposition: inline
In-Reply-To: <aed6cc9f-98f3-2693-ac81-52bb0052307e@2ndquadrant.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


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

Greetings,

* Ian Barwick (ian.barwick@2ndquadrant.com) wrote:
> Consider the following cascading standby setup with PostgreSQL 12:
>=20
> - there exists a running primary "A"
> - standby "B" is cloned from primary "A" using "pg_basebackup --write-rec=
overy-conf"
> - standby "C" is cloned from standby "B" using "pg_basebackup --write-rec=
overy-conf"
>=20
> So far, so good, everything works as expected.
>=20
> Now, for whatever reason, the user wishes standby "C" to follow another u=
pstream
> node (which one is not important here), so the user, in the comfort of th=
eir own psql
> command line (no more pesky recovery.conf editing!) issues the following:
>=20
>     ALTER SYSTEM SET primary_conninfo =3D 'host=3Dsomeothernode';
>=20
> and restarts the instance, and... it stays connected to the original upst=
ream node.
>=20
> Which is unexpected.
>=20
> Examining the the restarted instance, "SHOW primary_conninfo" still displ=
ays
> the original value for "primary_conninfo". Mysteriouser and mysteriouser.
>=20
> What has happened here is that with the option "--write-recovery-conf", P=
g12's
> pg_basebackup (correctly IMHO) appends the recovery settings to "postgres=
ql.auto.conf".
>=20
> However, on standby "C", pg_basebackup has dutifully copied over standby =
"B"'s
> existing "postgresql.auto.conf", which already contains standby "B"'s
> replication configuration, and appended standby "C"'s replication configu=
ration
> to that, which (before "ALTER SYSTEM" was invoked) looked something like =
this:
>=20
> 	# Do not edit this file manually!
> 	# It will be overwritten by the ALTER SYSTEM command.
> 	primary_conninfo =3D 'host=3Dnode_A'
> 	primary_conninfo =3D 'host=3Dnode_B'
>=20
> which is expected, and works because the last entry in the file is evalua=
ted, so
> on startup, standby "C" follows standby "B".
>=20
> However, executing "ALTER SYSTEM SET primary_conninfo =3D 'host=3Dsomeoth=
ernode'" left
> standby "C"'s "postgresql.auto.conf" file looking like this:
>=20
> 	# Do not edit this file manually!
> 	# It will be overwritten by the ALTER SYSTEM command.
> 	primary_conninfo =3D 'host=3Dsomeothernode'
> 	primary_conninfo =3D 'host=3Dnode_B'
>=20
> 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.

> As-is, the user will either need to repeatedly issue "ALTER SYSTEM RESET =
primary_conninfo"
> until the duplicates are cleared (which means "ALTER SYSTEM RESET ..." do=
esn't work as
> advertised, and is not an obvious solution anyway), or ignore the "Do not=
 edit this file manually!"
> warning and remove the offending entry/entries (which, if done safely, sh=
ould involve
> shutting down the instance first).
>=20
> Note this issue is not specific to pg_basebackup, primary_conninfo (or an=
y 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 effo=
rt (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.

> 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 actua=
lly not the case -
> as per replace_auto_config_value() ( src/backend/utils/misc/guc.c ):
>=20
>     /* Search the list for an existing match (we assume there's only one)=
 */
>=20
> the *first* match is replaced.
>=20
> Attached patch attempts to rectify this situation by having replace_auto_=
config_value()
> deleting any duplicate entries first, before making any changes to the la=
st 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..

> Arguably it might be sufficient (and simpler) to just scan the list for t=
he last
> entry, but removing preceding duplicates seems cleaner, as it's pointless
> (and a potential source of confusion) keeping entries around which will n=
ever be used.

I don't think we should only change the last entry, that seems like a
really bad idea.  I agree that we should clean up the file if we come
across it being invalid.

> Also attached is a set of TAP tests to check ALTER SYSTEM works as expect=
ed (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.

Thanks!

Stephen

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

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

iQIcBAEBCgAGBQJdA8aDAAoJEO1sijiDR2RVSUUP+gO7gWo5zUS6bCZztO/BjM5T
cpOwwSZ/RQsfE6v9AyvEPlsEPdG5kD1dmc7JN/8v35lPF5jRhsq+fLJfmpKMMEYt
tdLS3QXA5VupzikhKkm15agrFJpFeG4PWgpAfazbAseVS7cbbWEFKuYje5f2XGfM
hkPuH23isvvq9/zp48FaWLU39Y69gfKI1sSExZz8QQJZGUUq/OHg6ZnWKV6/myKA
9O0In96+djFMSy4bNHFyVteAap6yEfU42DaIbYuYSzJ0o6GvOvg6YlEkYWGG0Qwy
k/TgeUto5cdMOu/QBbb6JhFw/LglC4ynqoKOAKTtMMSneV9uXo66VuL+CznZr5Ag
ILkoCur5SCqzm0KrlPXrEx9XNqLhJBj8IicT235ZDbv8iTq4XqXZs9KLdcbYadrF
g2brCK/nIBuFvLcvBYAwaRdQAuSeFa57stoDL98wZ+a+m3qWpo2VxsRQP8fxtjZ7
/V2f0nrjC+2iQabVmgkonaxumu8HYWrRZsQ8/GLhHO8qVV8yWdqVjezrRna5yyuU
WWWbz9/nVBQCDttqqRihFomI8AXtudrI5Dr6clEFtNu16iFJ7tGlKSg7TEh7qarH
//ZfwZffSNNaPJLoEL+6cHZ+6huYjFr0d1l1FsihuyGu27nO6I5g8tyJUZe4rp8P
1gEa5XebE6WTvmsJW1In
=hLEn
-----END PGP SIGNATURE-----

--L5Ks5Ey27iMz7PUy--