At Mon, 30 Oct 2023 08:51:18 +0530, Amit Kapila <amit.kapil...@gmail.com> wrote
in
> I think we can simply change that error message to assert if we want
> to go with the check hook idea of yours. BTW, can we add
> GUC_check_errdetail() with a better message as some of the other check
> function uses? Also, I guess we can add some comments or at least
> refer to the existing comments to explain the reason of such a check.
Definitely. I've attached the following changes.
1. Added GUC_check_errdetail() to the check function.
2. Added a comment to the check function (based on my knowledge about
the feature).
3. Altered the ereport() into Assert() in
InvalidatePossiblyObsoleteSlot(). I considered removing the
!SlotIsLogical() condition since pg_upgrade always sets
max_slot_wal_keep_size to -1, but I left it unchanged.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 99823df3c7..f7dc966600 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -52,6 +52,7 @@
#include "storage/proc.h"
#include "storage/procarray.h"
#include "utils/builtins.h"
+#include "utils/guc_hooks.h"
/*
* Replication slot on-disk data structure.
@@ -111,6 +112,26 @@ static void RestoreSlotFromDisk(const char *name);
static void CreateSlotOnDisk(ReplicationSlot *slot);
static void SaveSlotToPath(ReplicationSlot *slot, const char *dir, int elevel);
+/*
+ * GUC check_hook for max_slot_wal_keep_size
+ *
+ * All WAL files on the publisher node must be retained during an upgrade to
+ * maintain connections from downstreams. While pg_upgrade explicitly sets
+ * this variable to -1, there are ways for users to modify it. Ensure it
+ * remains unchanged for safety. 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("\"max_slot_wal_keep_size\" must be set to
-1 during the upgrade.");
+ return false;
+ }
+ return true;
+}
+
/*
* Report shared-memory space needed by ReplicationSlotsShmemInit.
*/
@@ -1424,18 +1445,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/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);
