Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 2 Nov 2023 14:48:32 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Peter Smith <smithpb2250@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>,
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
	Julien Rouhaud <rjuju123@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: pg_upgrade and logical replication
Message-ID: <ZUM4MPXRk4KzI3C2@paquier.xyz>
References: <20230424072224.qd3awkfrjcanw2pa@jrouhaud>
 <TYAPR01MB5866FE3A990CAE1FFA3F1472F56A9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <CALDaNm1ZrbHaWpJwwNhDTJocRKWd3rEkgJazuDdZ9Z-WdvonFg@mail.gmail.com>
 <TYAPR01MB5866F1BA00D86EA7EA134348F5F1A@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <CALDaNm1JzqTreCUrhNu5E1gq7Q8r_u3+FrisyT7moOED=UdoCg@mail.gmail.com>
 <ZQvbV2sdzBY6WEBl@paquier.xyz>
 <CALDaNm0X1WnHot8YWWhpKmzwX-au+gqCSL9J3RicqvAmsm=u1Q@mail.gmail.com>
 <CAA4eK1+uLQ1AFzvQGXk_q8Oh_OoYxJoexuVP7aLocEuO8VzgjA@mail.gmail.com>
 <CALDaNm3ATayLn=YNO5PsXkKF15R_E8j-jEu5gGkv=Od8TBxzEA@mail.gmail.com>
 <CAHut+PsCzt=O3_xkyrskaZ3SMxaXoN4L5Z5CgvaGPNx3mXXxOQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="HLRn1Z8e0ukvn77a"
Content-Disposition: inline
In-Reply-To: 
 <CAHut+PsCzt=O3_xkyrskaZ3SMxaXoN4L5Z5CgvaGPNx3mXXxOQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZUM4MPXRk4KzI3C2%40paquier.xyz>
Precedence: bulk


--HLRn1Z8e0ukvn77a
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Nov 02, 2023 at 04:35:26PM +1100, Peter Smith wrote:
> The chance of being able to do so should be small as pg_upgrade uses its
> own port and unix domain directory (customizable as well with
> --socketdir), but just preventing the launcher to start is safer at the
> end, because we are then sure that no changes would ever be applied.
> ~
> "safer at the end" (??)

Well, just safer.

> 2a.
> The comment is one big long sentence. IMO it will be better to break it up.
> 2b.
> Add a blank line between this comment note and the previous one.

Yes, I found that equally confusing when looking at this patch, so
I've edited the patch this way when I was looking at it today.  This
is enough to do the job, so I have applied it for now, before moving
on with the second one of this thread.

> 2c.
> In a recent similar thread [1], they chose to implement a guc_hook to
> prevent a user from overriding this via the command line option during
> the upgrade. Shouldn't this patch do the same thing, for consistency?
> 2d.
> If you do implement such a guc_hook (per #2c above), then should the
> patch also include a test case for getting an ERROR if the user tries
> to override that GUC?

Yeah, that may be something to do, but I am not sure that it is worth
complicating the backend code for the remote case where one enforces
an option while we are already setting a GUC in the upgrade path:
https://www.postgresql.org/message-id/CAA4eK1Lh9J5VLypSQugkdD+H=_5-6p3rOocjo7JbTogcxA2hxg@mail.gmail.com

That feels like a lot of extra facility for cases that should never
happen.
--
Michael

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

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmVDODAACgkQnvQgOdby
QH193RAApdDtb0F/3G5F2Xm7hDPOPAUT1ruKBA/miMDGzdz/8ZqisG6rIuNcNugH
+E/rp8L+MekURrAc3vF0PTdX0vyas4I7lWXOsIMfYqvvsdZe6KXN/cWlHl1hkqwj
7FsYVnr2R+qnPgysV+v8TYjSs0neoxcDc4k5xM4OH3u+PRRHswshgya7kU8fbUPI
y0Q1Bd8zZTk6eeCNSaYspnjsqwy25L5NKP0w9pxeOKMYh8327uSy//s3WKf39END
ZHjR5XrS7U1jk2CjPhslpB2ABULqQf7a9r2SYvld9q6qoRf6BOqzaE0WLTqpR1sn
cXmeTEufghzeOfeK+TSwBJgoQ1pPrDueS1u/Gg32r6lMIhp+Y54nLfzh4Ufqw986
HKAOYoU7N34xvvfoD9mw45DV6nyaqMpMXuuJgvMoFpEMVy7V1QizHtsFnXEpE6ur
Iy/umn9XvhTv4S9O5AGODsGw8tDFQz1QO4lL8Mw24IZrBO8p5856q5p5uM6eJTes
K8crEyu42KTrIPUpa+YosgtR4TEHDrUY/71DbwjYxkj2ip6KE1sgCH16m+Hc+at6
lu65GdCqqm4uJEzlrfDAIa+FtDBAIuLhH3QFiDPYRMsCxIkdVartvgPoy5PqZJ4j
/eQcog1uEhRFBfxt6MlxJZHWe5TpzS25H+uyxQlv3CFF975C6bk=
=6gV9
-----END PGP SIGNATURE-----

--HLRn1Z8e0ukvn77a--