RE: pg_upgrade and logical replication

Dear Vignesh,

Thanks for updating the patch! Here are some comments.
They are mainly cosmetic because I have not read yours these days.

01. binary_upgrade_add_sub_rel_state()

```
+    /* We must check these things before dereferencing the arguments */
+    if (PG_ARGISNULL(0) || PG_ARGISNULL(1) || PG_ARGISNULL(2))
+        elog(ERROR, "null argument to binary_upgrade_add_sub_rel_state is not allowed")
```

But fourth argument can be NULL, right? I know you copied from other functions,
but they do not accept for all arguments. One approach is that pg_dump explicitly
writes InvalidXLogRecPtr as the fourth argument.

02. binary_upgrade_add_sub_rel_state()

```
+    if (!OidIsValid(relid))
+        ereport(ERROR,
+                errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                errmsg("invalid relation identifier used: %u", relid));
+
+    tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+    if (!HeapTupleIsValid(tup))
+        ereport(ERROR,
+                errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                errmsg("relation %u does not exist", relid))
```

I'm not sure they should be ereport(). Isn't it that they will be never occurred?
Other upgrade funcs do not have ereport(), and I think it does not have to be
translated.

03. binary_upgrade_replorigin_advance()

IIUC this function is very similar to pg_replication_origin_advance(). Can we
extract a common part of them? I think pg_replication_origin_advance() will be
just a wrapper, and binary_upgrade_replorigin_advance() will get the name of
origin and pass to it.

04. binary_upgrade_replorigin_advance()

Even if you do not accept 03, some variable name can be follow the function.

05. getSubscriptions()

```
+    appendPQExpBufferStr(query, "o.remote_lsn AS suboriginremotelsn\n")
```

Hmm, this value is taken anyway, but will be dumed only when the cluster is PG17+.
Should we avoid getting the value like subrunasowner and subpasswordrequired?
Not sure...

06. dumpSubscriptionTable()

Can we assert that remote version is PG17+?

07. check_for_subscription_state()

IIUC, this function is used only for old cluster. Should we follow
check_old_cluster_for_valid_slots()?

08. check_for_subscription_state()

```
+            fprintf(script, "database:%s subscription:%s schema:%s relation:%s state:%s not in required state\n",
+                    active_db->db_name,
+                    PQgetvalue(res, i, 0),
+                    PQgetvalue(res, i, 1),
+                    PQgetvalue(res, i, 2),
+                    PQgetvalue(res, i, 3));
```

IIRC, format strings should be double-quoted.

09. check_new_cluster_logical_replication_slots()

Checks for replication origin were added in check_new_cluster_logical_replication_slots(),
but I felt it became a super function. Can we devide?

10. check_new_cluster_logical_replication_slots()

Even if you reject above, it should be renamed.

11. pg_upgrade.h

```
+    int         subscription_count; /* number of subscriptions */
```

Based on other struct, it should be "nsubscriptions".

12. 004_subscription.pl

```
+use File::Path qw(rmtree);
```

I think this is not used.

13. 004_subscription.pl

```
+my $bindir = $new_sub->config_data('--bindir');
```
For extensibility, it might be better to separate for old/new bindir.

14. 004_subscription.pl

```
+my $synced_query =
+  "SELECT count(1) = 1 FROM pg_subscription_rel WHERE srsubstate = 'r'";
+$old_sub->poll_query_until('postgres', $synced_query)
+  or die "Timed out while waiting for subscriber to synchronize data";
```

Actually, I'm not sure it is really needed. wait_for_subscription_sync() in line 163
ensures that sync are done? Are there any holes around here?

15. 004_subscription.pl

```
+# Check the number of rows for each table on each server
+my $result =
+  $publisher->safe_psql('postgres', "SELECT count(*) FROM tab_upgraded");
+is($result, qq(50), "check initial tab_upgraded table data on publisher");
+$result =
+  $publisher->safe_psql('postgres', "SELECT count(*) FROM tab_not_upgraded");
+is($result, qq(1), "check initial tab_upgraded table data on publisher");
+$result =
+  $old_sub->safe_psql('postgres', "SELECT count(*) FROM tab_upgraded");
+is($result, qq(50),
+    "check initial tab_upgraded table data on the old subscriber");
+$result =
+  $old_sub->safe_psql('postgres', "SELECT count(*) FROM tab_not_upgraded");
+is($result, qq(0),
+    "check initial tab_not_upgraded table data on the old subscriber");
```

I'm not sure they are really needed. At that time pg_upgrade --check is called,
this won't change the state of clusters.

16. pg_proc.dat

```
+{ oid => '8404', descr => 'for use by pg_upgrade (relation for pg_subscription_rel)',
+  proname => 'binary_upgrade_add_sub_rel_state', proisstrict => 'f',
+  provolatile => 'v', proparallel => 'u', prorettype => 'void',
+  proargtypes => 'text oid char pg_lsn',
+  prosrc => 'binary_upgrade_add_sub_rel_state' },
+{ oid => '8405', descr => 'for use by pg_upgrade (remote_lsn for origin)',
+  proname => 'binary_upgrade_replorigin_advance', proisstrict => 'f',
+  provolatile => 'v', proparallel => 'u', prorettype => 'void',
+  proargtypes => 'text pg_lsn',
+  prosrc => 'binary_upgrade_replorigin_advance' },
```

Based on other function, descr just should be "for use by pg_upgrade".

Best Regards,
Hayato Kuroda
FUJITSU LIMITED