RE: [PoC] pg_upgrade: allow to upgrade publisher node

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

In response to

Responses

Browse pgsql-hackers by date

  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