RE: [PoC] pg_upgrade: allow to upgrade publisher node - Mailing list pgsql-hackers

Dear Vignesh,

Thank you for reviewing! New version can be available in [1].

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

Fixed.

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

Hmm, I felt current description seems sufficient, but how about the below?
"The old cluster has replicated all the transactions and logical decoding
 messages to subscribers."

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

I preferred to include all the needed items in each C files, but removed.

> 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);
> +       }
> +}

Fixed.

> 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);"
> +);

This part is removed because of the refactoring.

> 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");

Rephased.

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

I ran both.

[1]:
https://www.postgresql.org/message-id/TYAPR01MB5866AC8A7694113BCBE0A71EF5D6A%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED