Re: pg_upgrade and logical replication

Here are some review comments for patch v10-0001

======
Commit message

1.
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" (??)

======
src/bin/pg_upgrade/server.c

2.
+ * We don't want the launcher to run while upgrading because it may start
+ * apply workers which could start receiving changes from the publisher
+ * before the physical files are put in place, causing corruption on the
+ * new cluster upgrading to, so setting max_logical_replication_workers=0
+ * to disable launcher.
  */
  if (GET_MAJOR_VERSION(cluster->major_version) >= 1700)
- appendPQExpBufferStr(&pgoptions, " -c max_slot_wal_keep_size=-1");
+ appendPQExpBufferStr(&pgoptions, " -c max_slot_wal_keep_size=-1 -c
max_logical_replication_workers=0");

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.

~~~

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?

======
[1] https://www.postgresql.org/message-id/20231027.115759.2206827438943188717.horikyota.ntt%40gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia