Date: Mon, 24 Jun 2019 14:52:02 -0400
From: Stephen Frost <sfrost@snowman.net>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Ian Barwick <ian.barwick@2ndquadrant.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
Message-ID: <20190624185202.GV2480@tamriel.snowman.net>
References: <aed6cc9f-98f3-2693-ac81-52bb0052307e@2ndquadrant.com>
 <20190614160835.GF2480@tamriel.snowman.net>
 <CAA4eK1JW9UfLOtMEJq5b_YcogbWdxzAGUzNi72F2Xx4r1_h6QA@mail.gmail.com>
 <20190616171610.GO2480@tamriel.snowman.net>
 <27dc9f8d-8d8f-9dd1-0494-5d8bd2f49a45@2ndquadrant.com>
 <CA+TgmobVjrvVQB_gVytgBcPRmBvptN3wHQy5T6=dYCDZ5n5MGA@mail.gmail.com>
 <20190621152452.GN2480@tamriel.snowman.net>
 <CA+Tgmobv2kH0s9a81MkJ12hEOaRFzMka+5fc6=OTEf-X9pTiDQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="zFAswnri7mqJ3vEz"
Content-Disposition: inline
In-Reply-To: 
 <CA+Tgmobv2kH0s9a81MkJ12hEOaRFzMka+5fc6=OTEf-X9pTiDQ@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


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

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Jun 21, 2019 at 11:24 AM Stephen Frost <sfrost@snowman.net> wrote:
> > That's not quite accurate, given that it isn't how the ALTER SYSTEM call
> > itself works, and clearly isn't how the authors of that feature expected
> > things to work or they would have actually made it work.  They didn't,
> > and it doesn't actually work.
> >
> > The notion that pg_basebackup was correct in this, when it wasn't tested
> > at all, evidently, even after the concern was raised, and ALTER SYSTEM
> > was wrong, even though it worked just fine before some later patch
> > started making changes to the file, based on the idea that it's the
> > "natural approach" doesn't make sense to me.
> >
> > If the change to pg_basebackup had included a change to ALTER SYSTEM to
> > make it work the *same* way that pg_basebackup now does, or at least to
> > work with the changes that pg_basebackup were making, then maybe
> > everything would have been fine.
>=20
> This argument boils down to: two people patches don't play nicely
> together, and we should assume that the first patch had it right and
> the second patch had it wrong, because the first patch was first.

No, the point I was making is that one wasn't "natural" compared to the
other as we have two patches which clearly chose differently.  Had they
picked the same, as I said above, maybe everything would have been fine.

> I don't think it works like that. I think we should decide which patch
> had it right by looking at what the nicest behavior actually is, not
> by which one came first.  In my mind having ALTER SYSTEM drop
> duplicate that other tools may have introduced is a clear winner with
> basically no downside. You are arguing that it will produce confusion,
> but I don't really understand who is going to be confused or why they
> are going to be confused.  We can document whatever we do, and it
> should be fine.  Humans aren't generally supposed to be examining this
> file anyway, so they shouldn't get confused very often.

I'm not the only one who feels that it would be confusing for ALTER
SYSTEM to drop duplicates while every other tools creates them and
doesn't do anything to prevent them from being there.  As for who-
anyone who deals with PostgreSQL on a regular basis will end up running
into the "oh, huh, after pg_basebackup ran, I ended up with duplicates
in postgresql.auto.conf, I wonder if that's ok?" follow by "oh, errr, I
used to have duplicates but now they're gone?!?!  how'd that happen?",
unless, perhaps, they are reading this thread, in which case they'll
certainly know and expect it.  You can probably guess which camp is
larger.

When telling other tool authors how to manipulate PGDATA files, I really
dislike the "do as I say, not as I do" approach that you're advocating
for here.  Let's come up with a specific, clear, and ideally simple way
for everything to modify postgresql.auto.conf and let's have everything
use it.

> In my view, the original ALTER SYSTEM patch just has a bug -- it
> doesn't modify the right copy of the setting when multiple copies are
> present -- and we should just fix the bug.

Removing duplicates wouldn't be necessary for ALTER SYSTEM to just
modify the 'correct' version.

Thanks,

Stephen

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

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

iQIcBAEBCgAGBQJdERvSAAoJEO1sijiDR2RVQ9kP/3fyuhyFhRNIV491z2gecaKr
C2DSZNl7VcgbMGOrV4NjYCSrRvDZtEJJdgGh46FOVKl4xT6VnhtNxUFhkiJF1hsg
8rDBzMoz5RRdBBFikjrutxBYAdJKpWhAQ00IkJCxDe4ck5FHfeXwqI/AsNYlIxC5
PcGkuclZ6SODGw1wMDd719D469kXNuv3rGsvhjD2dNwxp7wbB/2ZIJjXE+KmebDN
A1Ikb2Si+BKMv0jBxWwS5zJEYGsUFsmVNPcLNupKaebPReD3FnCCz5b7WTv3BT8/
jh0OZpYYcw3iHn1AaUfU/jFxZaMIwMJEcoGI1NeNcyGvAq7GvnMnX9D2Wmr4C1Yp
n3+KUnTHRVlSNKVK9QQ6CYCi+xG6l0AS4nAnBO26984XPisv7ysqwoz0kARTS6VZ
bjCySZC61SCNT3ewJys8+iJ46nUqjYdAVsu9SOzVD7sCMO6dHFCY3N8A/uiDP3yU
m2Mc449RpLel02eFY1m3UACM1syXkzcDAUJdk94CFLxPx4GX7lOMzMiQcL8xL8Gm
VJBepOMnmPTiMMnJTDecLNr4nf7tfCdWr+8xn44v3ExOajuhgHJmfWbS2aJogkTO
0GmOcsfqOVvG0IZBXWht1/8tFp27h0Tff6Y82qDcCm1SRc8QAXTJp845vhSf4Q4r
WovWBJD7ZN34co17TDuA
=44bk
-----END PGP SIGNATURE-----

--zFAswnri7mqJ3vEz--