Re: pg_upgrade and logical replication

On Thu, 27 Apr 2023 at 13:18, Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)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