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/