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/