Re: [PoC] pg_upgrade: allow to upgrade publisher node

On Mon, 16 Oct 2023 at 14:44, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Sat, Oct 14, 2023 at 10:45 AM Hayato Kuroda (Fujitsu)
> <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> >
> > Here is a new patch.
> >
> > Previously I wrote:
> > > Based on above idea, I made new version patch which some functionalities were
> > > exported from pg_resetwal. In this approach, pg_upgrade itself removed WALs and
> > > then create logical slots, then pg_resetwal would be called with new option
> > > --no-switch, which avoid to switch a WAL segment file. The option is only used
> > > for the upgrading purpose so it is not written in doc and usage(). This option
> > > is not required if pg_resetwal -o does not discard WAL records. Please see the
> > > fork thread [1].
> >
> > But for now, these changes were reverted because changing pg_resetwal -o stuff
> > may be a bit risky. This has been located more than ten years so that we should
> > be more careful for modifying.
> > Also, I cannot come up with problems if slots are created after the pg_resetwal.
> > Background processes would not generate decodable changes (listed in [1]), and
> > BGworkers by extensions could be ignored [2].
> > Based on the discussion on forked thread [3] and if it is accepted, we will apply
> > again.
> >

1) Should this:
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+# Tests for upgrading replication slots
+
be:
"Tests for upgrading logical replication slots"

2) This statement is not entirely true:
+ <listitem>
+ <para>
+ The old cluster has replicated all the changes to subscribers.
+ </para>

If we have some changes like shutdown_checkpoint the upgrade passes,
if we have some changes like create view whose changes will not be
replicated the upgrade fails.

3) All these includes are not required except for "logical.h"
--- a/src/backend/utils/adt/pg_upgrade_support.c
+++ b/src/backend/utils/adt/pg_upgrade_support.c
@@ -11,14 +11,20 @@

#include "postgres.h"

+#include "access/xlogutils.h"
+#include "access/xlog_internal.h"
#include "catalog/binary_upgrade.h"
#include "catalog/heap.h"
#include "catalog/namespace.h"
#include "catalog/pg_type.h"
#include "commands/extension.h"
+#include "funcapi.h"
#include "miscadmin.h"
+#include "replication/logical.h"
+#include "replication/slot.h"
#include "utils/array.h"
#include "utils/builtins.h"
+#include "utils/pg_lsn.h"

4) We could print two_phase as true/false instead of 0/1:
+static void
+print_slot_infos(LogicalSlotInfoArr *slot_arr)
+{
+ /* Quick return if there are no logical slots. */
+ if (slot_arr->nslots == 0)
+ return;
+
+ pg_log(PG_VERBOSE, "Logical replication slots within the database:");
+
+ for (int slotnum = 0; slotnum < slot_arr->nslots; slotnum++)
+ {
+ LogicalSlotInfo *slot_info = &slot_arr->slots[slotnum];
+
+ pg_log(PG_VERBOSE, "slotname: \"%s\", plugin: \"%s\",
two_phase: %d",
+ slot_info->slotname,
+ slot_info->plugin,
+ slot_info->two_phase);
+ }
+}

5) test passes without the below, maybe this is not required:
+# 2. Consume WAL records to avoid another type of upgrade failure. It will be
+# tested in subsequent cases.
+$old_publisher->safe_psql('postgres',
+ "SELECT count(*) FROM
pg_logical_slot_get_changes('test_slot1', NULL, NULL);"
+);

6) This message "run of pg_upgrade of old cluster with idle
replication slots" seems wrong:
+# pg_upgrade will fail because the slot still has unconsumed WAL records
+command_checks_all(
+ [
+ 'pg_upgrade', '--no-sync',
+ '-d', $old_publisher->data_dir,
+ '-D', $new_publisher->data_dir,
+ '-b', $bindir,
+ '-B', $bindir,
+ '-s', $new_publisher->host,
+ '-p', $old_publisher->port,
+ '-P', $new_publisher->port,
+ $mode,
+ ],
+ 1,
+ [
+ qr/Your installation contains invalid logical
replication slots./
+ ],
+ [qr//],
+ 'run of pg_upgrade of old cluster with idle replication slots');
+ok( -d $new_publisher->data_dir . "/pg_upgrade_output.d",
+ "pg_upgrade_output.d/ not removed after pg_upgrade failure");

7) You could run pgindent and pgperlytidy, it shows there are few
issues present with the patch.

Regards,
Vignesh