On Thu, 2 Nov 2023 at 11:05, Peter Smith <smithpb2...@gmail.com> wrote:
>
> ~~~
>
> 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?
Added GUC hook 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?
Added a test for the same.
We can use this patch if we are planning to go ahead with guc_hooks
for max_slot_wal_keep_size as discussed at [1].
The attached patch has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CAHut%2BPsTrB%3DmjBA-Y-%2BW4kK63tao9%3DXBsMXG9rkw4g_m9WatwA%40mail.gmail.com
Regards,
Vignesh
From 00050247bad78b331dc1f841296dd40b3f37ecaf Mon Sep 17 00:00:00 2001
From: Vignesh C <vignes...@gmail.com>
Date: Fri, 3 Nov 2023 14:57:48 +0530
Subject: [PATCH] Added GUC hook for max_logical_replication_workers.
During a binary upgrade, pg_upgrade sets this variable to 0 via the command
line in an attempt to prevent startup of the logical replication launcher
which may start apply workers that could start receiving changes from the
publisher before the physical files are put in place, causing corruption on
the new cluster upgrading to, but users have ways to override it. Added
GUC hook to prevent overriding of max_logical_replication_workers
configuration.
---
src/backend/utils/init/postinit.c | 24 +++++++++++++++++
src/backend/utils/misc/guc_tables.c | 2 +-
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 36 ++++++++++++++++++++++++++
src/include/utils/guc_hooks.h | 2 ++
4 files changed, 63 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 552cf9d950..22e37d34e7 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -617,6 +617,30 @@ check_max_wal_senders(int *newval, void **extra, GucSource source)
return true;
}
+/*
+ * GUC check_hook for max_logical_replication_workers
+ *
+ * During a binary upgrade, pg_upgrade sets this variable to 0 via the command
+ * line in an attempt to prevent startup of the logical replication launcher
+ * which may start apply workers that could start receiving changes from the
+ * publisher before the physical files are put in place, causing corruption on
+ * the new cluster upgrading to, but users have ways to override it. To ensure
+ * the successful completion of the upgrade, it's essential to keep this
+ * variable unaltered. See start_postmaster() in pg_upgrade for more details.
+ */
+bool
+check_max_logical_replication_workers(int *newval, void **extra,
+ GucSource source)
+{
+ if (IsBinaryUpgrade && *newval)
+ {
+ GUC_check_errdetail("\"%s\" must be set to 0 during binary upgrade mode.",
+ "max_logical_replication_workers");
+ return false;
+ }
+ return true;
+}
+
/*
* Early initialization of a backend (either standalone or under postmaster).
* This happens even before InitPostgres.
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 7605eff9b9..4a5ff3d317 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -3049,7 +3049,7 @@ struct config_int ConfigureNamesInt[] =
},
&max_logical_replication_workers,
4, 0, MAX_BACKENDS,
- NULL, NULL, NULL
+ check_max_logical_replication_workers, NULL, NULL
},
{
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index c6d83d3c21..a6ca422c58 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -371,6 +371,42 @@ $oldnode->start;
$oldnode->safe_psql('postgres', 'DROP DATABASE regression_invalid');
$oldnode->stop;
+command_fails(
+ [
+ 'pg_upgrade', '--no-sync', '-d', $oldnode->data_dir,
+ '-D', $newnode->data_dir, '-b', $oldbindir,
+ '-B', $newbindir, '-s', $newnode->host,
+ '-p', $oldnode->port, '-P', $newnode->port,
+ '-O -c max_logical_replication_workers=1',
+ $mode, '--check',
+ ],
+ 'run of pg_upgrade with invalid max_logical_replication_workers');
+ok(-d $newnode->data_dir . "/pg_upgrade_output.d",
+ "pg_upgrade_output.d/ not removed after pg_upgrade failure");
+# Verify the reason why the logical replication slot cannot be upgraded
+my $upgrade_logfile;
+
+# Find pg_upgrade_server text file. We cannot predict the file's path because
+# the output directory contains a milliseconds timestamp.
+# File::Find::find must be used.
+find(
+ sub {
+ if ($File::Find::name =~ m/pg_upgrade_server\.log/)
+ {
+ $upgrade_logfile = $File::Find::name;
+ }
+ },
+ $newnode->data_dir . "/pg_upgrade_output.d");
+
+# Check that the server has logged a message saying server start failed with
+# invalid max_logical_replication_workers error.
+like(
+ slurp_file($upgrade_logfile),
+ qr/"max_logical_replication_workers\" must be set to 0 during binary upgrade mode/m,
+ 'cannot specify a different value to max_logical_replication_workers');
+
+rmtree($newnode->data_dir . "/pg_upgrade_output.d");
+
# --check command works here, cleans up pg_upgrade_output.d.
command_ok(
[
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index 2a191830a8..dda5651fa8 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -84,6 +84,8 @@ extern bool check_maintenance_io_concurrency(int *newval, void **extra,
extern void assign_maintenance_io_concurrency(int newval, void *extra);
extern bool check_max_connections(int *newval, void **extra, GucSource source);
extern bool check_max_wal_senders(int *newval, void **extra, GucSource source);
+extern bool check_max_logical_replication_workers(int *newval, void **extra,
+ GucSource source);
extern void assign_max_wal_size(int newval, void *extra);
extern bool check_max_worker_processes(int *newval, void **extra,
GucSource source);
--
2.34.1
