Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 5 Dec 2023 14:26:11 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>, Peter Smith <smithpb2250@gmail.com>,
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
	Julien Rouhaud <rjuju123@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: pg_upgrade and logical replication
Message-ID: <ZW60c-tpTNB5noL8@paquier.xyz>
References: 
 <CALDaNm1DpRq_Cx+KLkq6h7RtuAfVgt1EEJaXAbU_QrvHgf1evg@mail.gmail.com>
 <CAA4eK1K+59w1eb3vu4ZzZr0Ouf7tUHD146U1EWq++KH=BhiTpg@mail.gmail.com>
 <CALDaNm1-1Ong61cPmCOXMzDjhnRurtszwuss_gV0XrWjX8nG8A@mail.gmail.com>
 <CAA4eK1Kfs0fjRcGLv4RQsEfhRoWP+uRsKJ2NJUeTMrnOQMRCpQ@mail.gmail.com>
 <CALDaNm0ST1iSrJLD_CV6hQs=w4GZRCRdftQvQA3cO8Hq3QUvYw@mail.gmail.com>
 <CAA4eK1KmvGfWgrM7dm3W8PF623KKhKG5siNDZ-dNs-3UkPAFag@mail.gmail.com>
 <CALDaNm37E4tmSZd+k1ixtKevX3eucmhdOnw4pGmykZk4C1Nm4Q@mail.gmail.com>
 <CAHut+Pst-wJ-2nw90gAof+TbbfQC1ERsVeWvG_HaNH7xqu4Fdg@mail.gmail.com>
 <CALDaNm0U5QsyE0yJxt7rHkH-tO=0Q1Fu=PoueqL6boLGOUXN7g@mail.gmail.com>
 <CAA4eK1K-YGFOmS=Gi-GST0A5Xx4zBsoXn0TebDtZFNKUmjDPww@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="CWyHdjEWFGGmF+eL"
Content-Disposition: inline
In-Reply-To: 
 <CAA4eK1K-YGFOmS=Gi-GST0A5Xx4zBsoXn0TebDtZFNKUmjDPww@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZW60c-tpTNB5noL8%40paquier.xyz>
Precedence: bulk


--CWyHdjEWFGGmF+eL
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--CWyHdjEWFGGmF+eL
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmVutHMACgkQnvQgOdby
QH2TFQ/9FoPy0wtthxSwnntUapscn28R5Trc4Ikedk2U1wz8KFuBR+0sh13ZWb0i
/iCE5VEqtFGtr9K2xzxYjfqeYBhDd0k96p7m9RJHXHzS3WBw+f+Yh8l2YP8euqLk
XrDAFrmmNUIRoLkJIeFzehc5r8mzhg2m6oTNxb4/pRVZ/XIntQszyuA+74GNcOyn
wc2j3SQhOhIrxPGZ03P5A7ll/txgDcgpnhvUHOkvf6zqmBUQ3ihZ8qGX5GCCGj7W
jStzHXHt0kRt9hVl4pgXT8d5A0d4biv+re5gUOxzqNW5TfDF5KInn8D3ZyFIZa3O
VxgPD6HpLBhnF30LSLQesqOWT8gjUi3ysqjm1MAKE3qarU2XkbXzabpUEds4LVmB
UTWWUsmgDhk9iJLafS6bUG2D0bvrlP2+Z0bcyknzpnRtn7C7l/IYT8NQ7yCisoIm
yZaRWzSl2cYyBVyqDdpSiEtqw574BWaMPKJNgY39r/g/1C4RcOPdaC5ALAF/JGFQ
EJGDoP9nEBqcZIDZRQ5d8jXKzSrqD/mlWoSauqZyiA5ZgvSMQao6+rdq5FPKRgNQ
fgZaWG5o85RoVEaBkZPRjHDDL7TdpkqeWWPbATMSouAPq0T4Mt20sNw8g4P0Ic4x
QtDuoeL/sceQoxLLaJzCcPqcMX7YJvnIYT35X/xGcoutCIPijNA=
=FP75
-----END PGP SIGNATURE-----

--CWyHdjEWFGGmF+eL--