From: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
---|---|
To: | 'Amit Kapila' <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Bruce Momjian <bruce(at)momjian(dot)us>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
Subject: | RE: [PoC] pg_upgrade: allow to upgrade publisher node |
Date: | 2023-10-17 12:15:04 |
Message-ID: | TYAPR01MB5866AC8A7694113BCBE0A71EF5D6A@TYAPR01MB5866.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
v51-0001-pg_upgrade-Allow-to-replicate-logical-replicatio.patch | application/octet-stream | 45.9 KB |
v51-0002-support-cross-version-upgrade.patch | application/octet-stream | 18.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Hayato Kuroda (Fujitsu) | 2023-10-17 12:18:04 | RE: [PoC] pg_upgrade: allow to upgrade publisher node |
Previous Message | Amit Langote | 2023-10-17 11:12:22 | Re: remaining sql/json patches |