At Fri, 27 Oct 2023 14:57:10 +0530, Amit Kapila <[email protected]> wrote
in
> On Fri, Oct 27, 2023 at 2:02 PM Alvaro Herrera <[email protected]>
> wrote:
> >
> > On 2023-Oct-27, Kyotaro Horiguchi wrote:
> >
> > > @@ -1433,8 +1433,8 @@
> > > InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
> > > {
> > > 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"));
> >
> > Hmm, if I read this code right, this error is going to be thrown by the
> > checkpointer while finishing a checkpoint. Fortunately, the checkpoint
> > record has already been written, but I don't know what would happen if
> > this is thrown while trying to write the shutdown checkpoint. Probably
> > nothing terribly good.
> >
> > I don't think this is useful. If the setting is invalid during binary
> > upgrade, let's prevent it from being set at all right from the start of
> > the upgrade process.
>
> We are already forcing the required setting
> "max_slot_wal_keep_size=-1" during the upgrade similar to some of the
> other settings like "full_page_writes". However, the user can provide
> an option for "max_slot_wal_keep_size" in which case it will be
> overridden. Now, I think (a) we can ensure that our setting always
> takes precedence in this case. The other idea is (b) to parse the
> user-provided options and check if "max_slot_wal_keep_size" has a
> value different than expected and raise an error accordingly. Or we
> can simply (c) document the usage of max_slot_wal_keep_size in the
> upgrade. I am not sure whether it is worth complicating the code for
> this as the user shouldn't be using such an option during the upgrade.
> So, I think doing (a) and (c) could be simpler.
> >
> > In InvalidatePossiblyObsoleteSlot() we could have
> > just an Assert() or elog(PANIC).
> >
>
> Yeah, we can change to either of those.
This discussion seems like a bit off from my point. I suggested adding
a check for that setting when IsBinaryUpgraded is true at the GUC
level as shown in the attached patch. I believe Álvaro made a similar
suggestion. While the error message is somewhat succinct, I think it
is sufficient given the low possilibility of the scenario and the fact
that it cannot occur inadvertently.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 99823df3c7..b1495c4754 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,17 @@ 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
+ */
+bool
+check_max_slot_wal_keep_size(int *newval, void **extra, GucSource source)
+{
+ if (IsBinaryUpgrade && *newval != -1)
+ return false;
+ return true;
+}
+
/*
* Report shared-memory space needed by ReplicationSlotsShmemInit.
*/
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);
