Re: pg_upgrade and logical replication

On Thu, 27 Apr 2023 at 13:18, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Julien,
>
> Thank you for updating the patch! Followings are my comments.
>
> 01. documentation
>
> In this page steps to upgrade server with pg_upgrade is aligned. Should we write
> down about subscriber? IIUC, it is sufficient to just add to "Run pg_upgrade",
> like "Apart from streaming replication standby, subscriber node can be upgrade
> via pg_upgrade. At that time we strongly recommend to use --preserve-subscription-state".

Now this option has been removed and made default

> 02. AlterSubscription
>
> I agreed that oid must be preserved between nodes, but I'm still afraid that
> given oid is unconditionally trusted and added to pg_subscription_rel.
> I think we can check the existenec of the relation via SearchSysCache1(RELOID,
> ObjectIdGetDatum(relid)). Of cource the check is optional, so it should be
> executed only when USE_ASSERT_CHECKING is on. Thought?

Modified

> 03. main
>
> Currently --preserve-subscription-state and --no-subscriptions can be used
> together, but the situation is quite unnatural. Shouldn't we exclude them?

This option is removed now, so this scenario will not happen

> 04. getSubscriptionTables
>
>
> ```
> +       SubRelInfo *rels = NULL;
> ```
>
> The variable is used only inside the loop, so the definition should be also moved.

This logic is changed slightly, so it needs to be kept outside

> 05. getSubscriptionTables
>
> ```
> +                       nrels = atooid(PQgetvalue(res, i, i_nrels));
> ```
>
> atoi() should be used instead of atooid().

Modified

> 06. getSubscriptionTables
>
> ```
> +                       subinfo = findSubscriptionByOid(cur_srsubid);
> +
> +                       nrels = atooid(PQgetvalue(res, i, i_nrels));
> +                       rels = pg_malloc(nrels * sizeof(SubRelInfo));
> +
> +                       subinfo->subrels = rels;
> +                       subinfo->nrels = nrels;
> ```
>
> Maybe it never occurs, but findSubscriptionByOid() can return NULL. At that time
> accesses to their attributes will lead the Segfault. Some handling is needed.

This should not happen, added a fatal error in this case.

> 07. dumpSubscription
>
> Hmm, SubRelInfos are still dumped at the dumpSubscription(). I think this style
> breaks the manner of pg_dump. I think another dump function is needed. Please
> see dumpPublicationTable() and dumpPublicationNamespace(). If you have a reason
> to use the style, some comments to describe it is needed.

Modified

> 08. _SubRelInfo
>
> If you will address above comment, DumpableObject must be added as new attribute.

Modified

> 09. check_for_subscription_state
>
> ```
> +                       for (int i = 0; i < ntup; i++)
> +                       {
> +                               is_error = true;
> +                               pg_log(PG_WARNING,
> +                                          "\nWARNING:  subscription \"%s\" has an invalid remote_lsn",
> +                                          PQgetvalue(res, 0, 0));
> +                       }
> ```
>
> The second argument should be i to report the name of subscription more than 2.

Modified

> 10. 003_subscription.pl
>
> ```
> $old_sub->wait_for_subscription_sync($publisher, 'sub');
>
> my $result = $old_sub->safe_psql('postgres',
>     "SELECT COUNT(*) FROM pg_subscription_rel WHERE srsubstate != 'r'");
> is ($result, qq(0), "All tables in pg_subscription_rel should be in ready state");
> ```
>
> I think there is a possibility to cause a timing issue, because the SELECT may
> be executed before srsubstate is changed from 's' to 'r'. Maybe poll_query_until()
> can be used instead.

Modified

> 11. 003_subscription.pl
>
> ```
> command_ok(
>         [
>                 'pg_upgrade', '--no-sync',        '-d', $old_sub->data_dir,
>                 '-D',         $new_sub->data_dir, '-b', $bindir,
>                 '-B',         $bindir,            '-s', $new_sub->host,
>                 '-p',         $old_sub->port,     '-P', $new_sub->port,
>                 $mode,
>                 '--preserve-subscription-state',
>                 '--check',
>         ],
>         'run of pg_upgrade --check for old instance with correct sub rel');
> ```
>
> Missing check of pg_upgrade_output.d?

Modified

> And maybe you missed to run pgperltidy.

It has been run for the new patch.

The attached v7 patch has the changes for the same.

Regards,
Vignesh