At Fri, 27 Oct 2023 14:57:10 +0530, Amit Kapila <amit.kapil...@gmail.com> wrote in > On Fri, Oct 27, 2023 at 2:02 PM Alvaro Herrera <alvhe...@alvh.no-ip.org> > 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);