Dear Amit, Thanks for reviewing! PSA new version.
>
> Yeah, I think introducing additional complexity unless it is really
> required sounds a bit scary to me as well. BTW, please find attached
> some cosmetic changes.
Basically LGTM, but below part was conflicted with Bharath's comment [1].
```
@@ -1607,7 +1605,7 @@ check_old_cluster_for_valid_slots(bool live_check)
fclose(script);
pg_log(PG_REPORT, "fatal");
- pg_fatal("Your installation contains logical replication slots
that cannot be upgraded.\n"
+ pg_fatal("Your installation contains invalid logical
replication slots.\n"
```
How about " Your installation contains logical replication slots that can't be
upgraded."?
> One minor additional comment:
> +# Initialize subscriber cluster
> +my $subscriber = PostgreSQL::Test::Cluster->new('subscriber');
> +$subscriber->init(allows_streaming => 'logical');
>
> Why do we need to set wal_level as logical for subscribers?
It is not mandatory. The line was copied from tests in src/test/subscription.
Removed the setting from my patch. I felt that it could be removed from other
patches. I will fork new thread and post the patch.
Also, I did some improvements based on the v50, basically for tests.
1. Test file was refactored. pg_uprade was executed many times in the test so
the
test time was increasing. Below refactorings were done.
===
a. Checks for both transactional and non-transactional changes were done at the
same time.
b. Removed the dry-run test. It did not improve the coverage.
c. Removed the wal_level test. Other tests like subscriptions and test_decoding
do not contain test for GUCs, so I thought it could be acceptable. Removing
all the GUC test (for max_replication_slots) might be risky, so it was
remained.
===
2. Supported the cross-version checks. If an environment variable "oldinstall"
is set, use the binary as old cluster. If the specified one is PG16-, the
test verifies that logical replication slots would not be migrated.
002_pg_upgrade.pl requires that $ENV(olddump) must be also defined, but it
is not needed for our test. I tried to support from PG9.2, which is the
oldest
version for Xupgrade test [2]. You can see 0002 patch for it.
IIUC pg_create_logical_replication_slot() can be available since PG9.4, so
tests
will be skipped if older executables are specified, like:
```
$ oldinstall=/home/hayato/older/pg92/ make check
PROVE_TESTS='t/003_upgrade_logical_replication_slots.pl'
...
# +++ tap check in src/bin/pg_upgrade +++
t/003_upgrade_logical_replication_slots.pl .. skipped: Logical replication
slots can be available since PG9.4
Files=1, Tests=0, 0 wallclock secs ( 0.03 usr 0.00 sys + 0.08 cusr 0.02
csys = 0.13 CPU)
Result: NOTESTS
```
[1]:
https://www.postgresql.org/message-id/CALj2ACXp%2BLXioY_%3D9mboEbLD--4c4nnpJCZ%2Bj4fckBdSOQhENA%40mail.gmail.com
[2]:
https://github.com/PGBuildFarm/client-code/releases#:~:text=support%20for%20testing%20cross%20version%20upgrade%20extended%20back%20to%209.2
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
v51-0001-pg_upgrade-Allow-to-replicate-logical-replicatio.patch
Description: v51-0001-pg_upgrade-Allow-to-replicate-logical-replicatio.patch
v51-0002-support-cross-version-upgrade.patch
Description: v51-0002-support-cross-version-upgrade.patch
