From: | Michael Paquier <michael(at)paquier(dot)xyz> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | vignesh C <vignesh21(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: pg_upgrade and logical replication |
Date: | 2023-12-05 05:26:11 |
Message-ID: | ZW60c-tpTNB5noL8@paquier.xyz |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Dec 04, 2023 at 04:30:49PM +0530, Amit Kapila wrote:
> I have made minor changes in the comments and code at various places.
> See and let me know if you are not happy with the changes. I think
> unless there are more suggestions or comments, we can proceed with
> committing it.
Yeah. I am planning to look more closely at what you have here, and
it is going to take me a bit more time though (some more stuff planned
for next CF, an upcoming conference and end/beginning-of-year
vacations), but I think that targetting the beginning of next CF in
January would be OK.
Overall, I have the impression that the patch looks pretty solid, with
a restriction in place for "init" and "ready" relations, while there
are tests to check all the states that we expect. Seeing coverage
about all that makes me a happy hacker.
+ * If retain_lock is true, then don't release the locks taken in this function.
+ * We normally release the locks at the end of transaction but in binary-upgrade
+ * mode, we expect to release those immediately.
I think that this should be documented in pg_upgrade_support.c where
the caller expects the locks to be released, and why these should be
released. There is a risk that this comment becomes obsolete if
AddSubscriptionRelState() with locks released is called in a different
code path. Anyway, I am not sure to get why this is OK, or even
necessary. It seems like a good practice to keep the locks on the
subscription until the transaction that updates its state. If there's
a specific reason explaining why that's better, the patch should tell
why.
+ * However, this shouldn't be a problem as the upgrade ensures
+ * that all the transactions were replicated before upgrading the
+ * publisher.
This wording looks a bit confusing to me, as "the upgrade" could refer
to the upgrade of a subscriber, but what we want to tell is that the
replay of the transactions is enforced when doing a publisher upgrade.
I'd suggest something like "the upgrade of the publisher ensures that
all the transactions were replicated before upgrading it".
+my $result = $old_sub->safe_psql('postgres',
+ "SELECT count(1) = 1 FROM pg_subscription_rel WHERE srsubstate = 'i'");
+is($result, qq(t), "Check that the table is in init state");
Hmm. Not sure that this is safe. Shouldn't this be a
poll_query_until(), polling that the state of the relation is what we
want it to be after requesting a fresh of the publication on the
subscriber?
--
Michael
From | Date | Subject | |
---|---|---|---|
Next Message | Hayato Kuroda (Fujitsu) | 2023-12-05 05:41:09 | RE: [PoC] pg_upgrade: allow to upgrade publisher node |
Previous Message | Shubham Khanna | 2023-12-05 05:22:43 | Re: [DOCS] HOT - correct claim about indexes not referencing old line pointers |