Re: A recent message added to pg_upgade

At Mon, 30 Oct 2023 14:55:01 +0530, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote in
> > I get it. I agree to go with just the assert because the GUC
> > check_hook kinda tightens the screws against setting
> > max_slot_wal_keep_size to a value other than -1 during the binary
> > upgrade,

Thanks for being on the same page.

> A few comments on your inhibit_m_s_w_k_s_during_upgrade_2.txt:
>
> 1.
> + * All WAL files on the publisher node must be retained during an upgrade to
> + * maintain connections from downstreams. While pg_upgrade explicitly sets
>
> It's not just the publisher, anyone using logical slots. Also, no
> downstream please. If you want, you can repurpose the comment that's
> added by 29d0a77f.
>
> /*
> * Use max_slot_wal_keep_size as -1 to prevent the WAL removal by the
> * checkpointer process. If WALs required by logical replication slots
> * are removed, the slots are unusable. This setting prevents the
> * invalidation of slots during the upgrade. We set this option when
> * cluster is PG17 or later because logical replication slots can only be
> * migrated since then. Besides, max_slot_wal_keep_size is added in PG13.
> */

It is helpful. Thanks!

> 2.
> At present, only logical slots really require
> + * this.
>
> Can we remove the above comment as the code with SlotIsLogical(s)
> explains it all?

max_slot_wal_keep_size affects both logical and physical
slots. Therefore, if we are interested only one of the two types of
slots, it's important to clarify the rationale. Regardless of any
potential exntension to physical slots, I believe it's essential to
clarify the rationale. I couldn't determine from that extensive thread
whether there's a possible extension to physical slots. Could you
inform me if such an extension can happen and, if not, provide the
reason?

> 3.
> + GUC_check_errdetail("\"max_slot_wal_keep_size\" must be set
> to -1 during the upgrade.");
> + return false;
>
> How about we be explicit like the following which helps users reason
> about this restriction instead of them looking at the comments/docs?
>
> 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, I think the
errdetail message might potentially confuse those whosee it. Adding
"binary" looks fine as is and done in the attached.

> 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.

> 5. I think the check_hook is better defined in xlog.c the place where
> it's actually being declared and in action. IMV, there's no reason for
> it to be in slot.c as it doesn't deal with any slot related
> variables/structs. This also avoids an unnecessary "utils/guc_hooks.h"
> inclusion in slot.c.
> +bool
> +check_max_slot_wal_keep_size(int *newval, void **extra, GucSource source)
> +{

Sounds reasonable. Moved. I simply moved it to xlog.c, but the
function comment was thoroughly written only for this moved function,
making it somewhat stand out..

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

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center