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".
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?
03. main
Currently --preserve-subscription-state and --no-subscriptions can be used
together, but the situation is quite unnatural. Shouldn't we exclude them?
04. getSubscriptionTables
```
+ SubRelInfo *rels = NULL;
```
The variable is used only inside the loop, so the definition should be also
moved.
05. getSubscriptionTables
```
+ nrels = atooid(PQgetvalue(res, i, i_nrels));
```
atoi() should be used instead of atooid().
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.
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.
08. _SubRelInfo
If you will address above comment, DumpableObject must be added as new
attribute.
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.
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.
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?
And maybe you missed to run pgperltidy.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
