Re: A recent message added to pg_upgade

On Tue, Oct 31, 2023 at 2:19 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> >             GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE);
> >             GUC_check_errmsg(""\"max_slot_wal_keep_size\" must be set
> > to -1 when in binary upgrade mode");
> >             GUC_check_errdetail("A value of -1 prevents the removal of
> > WAL required for logical slots upgrade.");
> >             return false;
>
> I don't quite see the reason to provide such a detailed explanation
> just for this error. Additionally, since this check is performed
> regardless of the presence or absense of logical slots.

Okay, I get it.

> > 4. I think a test case to hit the error in the check hook in
> > 003_logical_slots.pl will help a lot here - not only covers the code
> > but also helps demonstrate how one can reach the error.
>
> Yeah, of course. I was planning to add tests once the direction of the
> discussion became clear. I will add them in the next version.

Yes, please. The test case to hit the ERROR in
InvalidatePossiblyObsoleteSlot() is important even if the check_hook
approach isn't going anywhere.

> function comment was thoroughly written only for this moved function,
> making it somewhat stand out..

I think that's fine.

> > 5. A possible problem with this check_hook approach is that it doesn't
> > let anyone setting max_slot_wal_keep_size to a value other than -1
> > during pg_ugprade even if someone doesn't have logical slots or
> > doesn't want to upgrade logical slots in which case the WAL file
> > growth during pg_upgrade may be huge (transiently) unless the
> > pg_resetwal step of pg_upgrade removes it at the end.
>
> While I doubt anyone wishes to set the variable to a specific value
> during upgrade, think there are individuals who might be reluctant to
> edit the config file due to unclear reasons.  While we could consider
> an alternative - checking for logical slots during binary upgrade-
> it's debatable if the effort is justified. (I haven't verified its
> feasibility, however.)

Checking for logical slots during binary upgrade doesn't help - what
if there are logical slots present but no upgrade is wanted (via a new
pg_uprade option)? Basically, how will the postgres server know
whether someone wants pg_upgrade of logical slots or not? Can we check
if someone is overriding max_slot_wal_keep_size in pg_upgrade itself
(via pg_settings  query from the server)? If yes, if logical slots
exist and upgrade is wanted, then disallow the upgrade if GUC is set
to value other than -1.

I believe disallowing setting max_slot_wal_keep_size to a value other
than -1 during binary upgrade may have serious consequences as it
impacts WAL retention before the pg_resetwal comes into picture as
part of pg_upgrade.

Or what if we just live with what we have right now? I mean with ERROR
in InvalidatePossiblyObsoleteSlot().

Or what if we just remove ERROR in InvalidatePossiblyObsoleteSlot or
make it an Assert and say do not override max_slot_wal_keep_size in
docs? Even if someone did override, let the pg_upgrade report the slot
as invalidated and let the user delete the slot or decide what to do
with it.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com