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
