Re: pg_upgrade and logical replication

Here are some review comments for the v4-0001 test code only.

======

1.
All the comments look alike, so it is hard to know what is going on.
If each of the main test parts could be highlighted then the test code
would be easier to read IMO.

Something like below:

# ==========
# TEST CASE: Check that pg_upgrade refuses to upgrade a subscription
when the replication origin is not set.
#
# replication origin's remote_lsn isn't set if data was not replicated after the
# initial sync.

...

# ==========
# TEST CASE: Check that pg_upgrade refuses to upgrade a subscription
with non-ready tables.

...

# ==========
# TEST CASE: Check that pg_upgrade works when all subscription tables are ready.

...

# ==========
# TEST CASE: Change the publication while the old subscriber is offline.
#
# Stop the old subscriber, insert a row in each table while it's down, and add
# t2 to the publication.

...

# ==========
# TEST CASE: Enable the subscription.

...

# ==========
# TEST CASE: Refresh the subscription to get the newly published table t2.
#
# Only the missing row on t2 show be replicated.

~~~

2.
+# replication origin's remote_lsn isn't set if not data is replicated after the
+# initial sync

wording:
/if not data is replicated/if data is not replicated/

~~~

3.
# Make sure the replication origin is set

I was not sure if all of the SELECT COUNT(*) checking is needed
because it just seems normal pub/sub functionality. There is no
pg_upgrade happening, so really it seemed the purpose of this part was
mainly to set the origin so that it will not be a blocker for
ready-state tests that follow this code. Maybe this can just be
incorporated into the following test part.

~~~

4.
# There should be no new replicated rows before enabling the subscription
$result = $new_sub->safe_psql('postgres',
"SELECT count(*) FROM t1");
is ($result, qq(2), "Table t1 should still have 2 rows on the new subscriber");

4a.
TBH, I felt it might be easier to follow if the SQL was checking for
WHERE (text = "while old_sub is down") etc, rather than just using
SELECT COUNT(*), and then trusting the comments to describe what the
different counts mean.

~

4b.
All these messages like "Table t1 should still have 2 rows on the new
subscriber" don't seem very helpful. e.g. They are not saying anything
about WHAT this is testing or WHY it should still have 2 rows.

~~~

5.
# Refresh the subscription, only the missing row on t2 show be replicated

/show/should/

------
Kind Regards,
Peter Smith.
Fujitsu Australia.