Re: A recent message added to pg_upgade

On Mon, Oct 30, 2023 at 2:31 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>

Never mind. Previous message was accidentally sent before I finished
writing my comments.

> Yeah. The check_hook is called even after the param is specified in
> postgresql.conf during the upgrade, so I see no problem there.
>
> > The error message, which is deemed impossible, adds an additional
> > message translation. In another thread, we are discussing the
> > reduction of translatable messages. Therefore, I suggest using elog()
> > for the condition at the very least. Whether it should be elog() or
> > Assert() remains open for discussion, as I don't have a firm stance on
> > it.
>
> 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,

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

2.
At present, only logical slots really require
+ * this.

Can we remove the above comment as the code with SlotIsLogical(s)
explains it all?

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;

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.

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)
+{

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.

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