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
