On Wed, Sep 20, 2023 at 09:38:56AM +0900, Michael Paquier wrote: > And this code path is used to start postmaster instances for old and > new clusters. So it seems to me that it is incorrect if this is not > conditional based on the cluster version.
Avoiding the startup of bgworkers during pg_upgrade is something that worries me a bit, actually, as it could be useful in some cases like monitoring? That would be fancy, for sure.. For now and seeing a lack of consensus on this larger matter, I'd like to propose a check for IsBinaryUpgrade into ApplyLauncherRegister() instead as it makes no real sense to start apply workers in this context. That would be equivalent to max_logical_replication_workers = 0. Amit, Vignesh, would the attached be OK for both of you? (Vignesh has posted a slightly different version of this patch on a different thread, but the subscriber part should be part of this thread with the subscribers, I assume.) -- Michael
From 2df408695163eb46bfe7efa9a9ccc07ff5fab183 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 25 Sep 2023 10:54:59 +0900 Subject: [PATCH] Prevent startup of logical replication launcher in binary upgrade mode The logical replication launcher may start apply workers during an upgrade, which could be the cause of corruptions on a new cluster if these are able to apply changes before the physical files are copied over. 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. Author: Vignesh C Discussion: https://postgr.es/m/CALDaNm2g9ZKf=y8X6z6MsLCuh8WwU-=q6plj35nfi2m5bzn...@mail.gmail.com --- src/backend/replication/logical/launcher.c | 9 +++++++++ src/bin/pg_upgrade/server.c | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c index 7882fc91ce..9c610edbeb 100644 --- a/src/backend/replication/logical/launcher.c +++ b/src/backend/replication/logical/launcher.c @@ -925,6 +925,15 @@ ApplyLauncherRegister(void) { BackgroundWorker bgw; + /* + * We don't want the launcher to run in binary upgrade mode 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. + */ + if (IsBinaryUpgrade) + return; + if (max_logical_replication_workers == 0) return; diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c index 0bc3d2806b..edbc101269 100644 --- a/src/bin/pg_upgrade/server.c +++ b/src/bin/pg_upgrade/server.c @@ -228,7 +228,7 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error) #endif /* - * Use -b to disable autovacuum. + * Use -b to disable autovacuum and logical replication launcher. * * Turn off durability requirements to improve object creation speed, and * we only modify the new cluster, so only use it there. If there is a -- 2.40.1
signature.asc
Description: PGP signature
