Re: A recent message added to pg_upgade

On 2023-Nov-02, Kyotaro Horiguchi wrote:

> 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;
> +}

One sentence in that comment reads weird. I'd do this:

s/To ensure the ... unaltered/This check callback ensures the value is
not overridden by the user/

> 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));

I think it's worth adding a comment here, pointing to
check_old_cluster_for_valid_slots() verifying that no
already-invalidated slots exist before the upgrade starts.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/