Dear Horiguchi-san, hackers,
> Thanks you for the comments!
Thanks for updating the patch!
I'm not sure it is intentional, but you might miss my post...I suggested to add
a
testcase.
I attached the updated version which is almost the same as Horiguchi-san's one,
but has a test. How do you think? Do you have other idea for testing?
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index b541be8eec..46833f6ecd 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2063,6 +2063,29 @@ check_wal_segment_size(int *newval, void **extra,
GucSource source)
return true;
}
+/*
+ * GUC check_hook for max_slot_wal_keep_size
+ *
+ * If WALs needed by logical replication slots are deleted, these slots become
+ * inoperable. During a binary upgrade, pg_upgrade sets this variable to -1 via
+ * the command line in an attempt to prevent such deletions, but users have
+ * ways to override it. To ensure the successful completion of the upgrade,
+ * it's essential to keep this variable unaltered. See
+ * InvalidatePossiblyObsoleteSlot() and start_postmaster() in pg_upgrade for
+ * more details.
+ */
+bool
+check_max_slot_wal_keep_size(int *newval, void **extra, GucSource source)
+{
+ if (IsBinaryUpgrade && *newval != -1)
+ {
+ GUC_check_errdetail("\"%s\" must be set to -1 during binary
upgrade mode.",
+ "max_slot_wal_keep_size");
+ return false;
+ }
+ return true;
+}
+
/*
* At a checkpoint, how many WAL segments to recycle as preallocated future
* XLOG segments? Returns the highest segment that should be preallocated.
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 99823df3c7..5c3d2b1082 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1424,18 +1424,12 @@
InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
SpinLockRelease(&s->mutex);
/*
- * The logical replication slots shouldn't be invalidated as
- * max_slot_wal_keep_size GUC is set to -1 during the upgrade.
- *
- * The following is just a sanity check.
+ * check_max_slot_wal_keep_size() ensures
max_slot_wal_keep_size is set
+ * to -1, so, slot invalidation for logical slots shouldn't
happen
+ * during an upgrade. At present, only logical slots really
require
+ * this.
*/
- if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)
- {
- ereport(ERROR,
-
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("replication slots must not be
invalidated during the upgrade"),
- errhint("\"max_slot_wal_keep_size\"
must be set to -1 during the upgrade"));
- }
+ Assert (!(*invalidated && SlotIsLogical(s) && IsBinaryUpgrade));
if (active_pid != 0)
{
diff --git a/src/backend/utils/misc/guc_tables.c
b/src/backend/utils/misc/guc_tables.c
index 7605eff9b9..818ffdb2ae 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2845,7 +2845,7 @@ struct config_int ConfigureNamesInt[] =
},
&max_slot_wal_keep_size_mb,
-1, -1, MAX_KILOBYTES,
- NULL, NULL, NULL
+ check_max_slot_wal_keep_size, NULL, NULL
},
{
diff --git a/src/bin/pg_upgrade/t/003_logical_slots.pl
b/src/bin/pg_upgrade/t/003_logical_slots.pl
index 5b01cf8c40..1a55a75827 100644
--- a/src/bin/pg_upgrade/t/003_logical_slots.pl
+++ b/src/bin/pg_upgrade/t/003_logical_slots.pl
@@ -22,7 +22,36 @@ $oldpub->init(allows_streaming => 'logical');
my $newpub = PostgreSQL::Test::Cluster->new('newpub');
$newpub->init(allows_streaming => 'logical');
-# Setup a common pg_upgrade command to be used by all the test cases
+# In a VPATH build, we'll be started in the source directory, but we want
+# to run pg_upgrade in the build directory so that any files generated finish
+# in it, like delete_old_cluster.{sh,bat}.
+chdir ${PostgreSQL::Test::Utils::tmp_check};
+
+# ------------------------------
+# TEST: Confirm max_slot_wal_keep_size must not be overwritten
+
+# pg_upgrade will fail because the GUC max_slot_wal_keep_size is overwritten
+# to a positive value
+command_checks_all(
+ [
+ 'pg_upgrade', '--no-sync',
+ '-d', $oldpub->data_dir,
+ '-D', $newpub->data_dir,
+ '-b', $oldpub->config_data('--bindir'),
+ '-B', $newpub->config_data('--bindir'),
+ '-s', $newpub->host,
+ '-p', $oldpub->port,
+ '-P', $newpub->port,
+ $mode, '-o " -c max_slot_wal_keep_size=1MB"'
+ ],
+ 1,
+ [qr/could not connect to source postmaster started with the command/],
+ [qr//],
+ 'run of pg_upgrade where max_slot_wal_keep_size is overwritten.');
+ok(-d $newpub->data_dir . "/pg_upgrade_output.d",
+ "pg_upgrade_output.d/ not removed after pg_upgrade failure");
+
+# Setup a common pg_upgrade command to be used by upcoming test cases
my @pg_upgrade_cmd = (
'pg_upgrade', '--no-sync',
'-d', $oldpub->data_dir,
@@ -34,11 +63,6 @@ my @pg_upgrade_cmd = (
'-P', $newpub->port,
$mode);
-# In a VPATH build, we'll be started in the source directory, but we want
-# to run pg_upgrade in the build directory so that any files generated finish
-# in it, like delete_old_cluster.{sh,bat}.
-chdir ${PostgreSQL::Test::Utils::tmp_check};
-
# ------------------------------
# TEST: Confirm pg_upgrade fails when the new cluster has wrong GUC values
@@ -67,8 +91,6 @@ command_checks_all(
[qr//],
'run of pg_upgrade where the new cluster has insufficient
max_replication_slots'
);
-ok( -d $newpub->data_dir . "/pg_upgrade_output.d",
- "pg_upgrade_output.d/ not removed after pg_upgrade failure");
# Set 'max_replication_slots' to match the number of slots (2) present on the
# old cluster. Both slots will be used for subsequent tests.
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index 2a191830a8..3d74483f44 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_slot_wal_keep_size(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);
