Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 18 Mar 2024 15:08:02 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Nathan Bossart <nathandbossart@gmail.com>,
	Japin Li <japinli@hotmail.com>,
	Ian Lawrence Barwick <barwick@gmail.com>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	Cary Huang <cary.huang@highgo.ca>,
	SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Switching XLog source from archive to streaming when primary
 available
Message-ID: <ZffaQt7UbM2Q9kYh@paquier.xyz>
References: 
 <ME3P282MB3166BDEACEFC8E4DFFE8B157B6512@ME3P282MB3166.AUSP282.PROD.OUTLOOK.COM>
 <CALj2ACU9O0q_JiGbtjPY4Ue2ax4aFH+x5Htw2nwOXQi0=p4ouQ@mail.gmail.com>
 <ME3P282MB3166D3391A9FC11DD35C05D9B6502@ME3P282MB3166.AUSP282.PROD.OUTLOOK.COM>
 <CALj2ACXV9=q5mXbPTgk9gKPQroxjpgyP7VKQnA1FREQO0iRXiA@mail.gmail.com>
 <20240305020452.GA3373526@nathanxps13>
 <CALj2ACVYVgq+QxeLmhY5DO+D6+7PwerdsVTeBxPuSeFyrgOPtg@mail.gmail.com>
 <20240305195212.GB3481820@nathanxps13>
 <CALj2ACVict-k-r94VYkJ3RB8=b-=hotS7N=e2_PmndFEke2r8w@mail.gmail.com>
 <20240306161926.GB3542434@nathanxps13>
 <CALj2ACU32_N+ymdW01WHTP+xEr8yVi9u1sFoqqg3kL4u36Mx5A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="eq3/hf7y7cv/yunH"
Content-Disposition: inline
In-Reply-To: 
 <CALj2ACU32_N+ymdW01WHTP+xEr8yVi9u1sFoqqg3kL4u36Mx5A@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZffaQt7UbM2Q9kYh%40paquier.xyz>
Precedence: bulk


--eq3/hf7y7cv/yunH
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sun, Mar 17, 2024 at 11:37:58AM +0530, Bharath Rupireddy wrote:
> Rebase needed after 071e3ad59d6fd2d6d1277b2bd9579397d10ded28 due to a
> conflict in meson.build. Please see the attached v23 patch.

I've been reading this patch, and this is a very tricky one.  Please
be *very* cautious.

+#streaming_replication_retry_interval =3D 0    # time after which standby
+                    # attempts to switch WAL source from archive to
+                    # streaming replication in seconds; 0 disables

This stuff allows a minimal retry interval of 1s.  Could it be useful
to have more responsiveness here and allow lower values than that?
Why not switching the units to be milliseconds?

+    if (streaming_replication_retry_interval <=3D 0 ||
+        !StandbyMode ||
+        currentSource !=3D XLOG_FROM_ARCHIVE)
+        return SWITCH_TO_STREAMING_NONE;

Hmm.  Perhaps this should mention why we don't care about the
consistent point.

+                /* See if we can switch WAL source to streaming */
+                if (wal_source_switch_state =3D=3D SWITCH_TO_STREAMING_NON=
E)
+                    wal_source_switch_state =3D SwitchWALSourceToPrimary();

Rather than a routine that returns as result the value to use for the
GUC, I'd suggest to let this routine set the GUC as there is only one
caller of SwitchWALSourceToPrimary().  This can also include a check
on SWITCH_TO_STREAMING_NONE, based on what I'm reading that.

-       if (lastSourceFailed)
+       if (lastSourceFailed ||
+           wal_source_switch_state =3D=3D SWITCH_TO_STREAMING)=20

Hmm.  This one may be tricky.  I'd recommend a separation between the
failure in reading from a source and the switch to a new "forced"
source.

+                if (wal_source_switch_state =3D=3D SWITCH_TO_STREAMING_PEN=
DING)
+                    readFrom =3D XLOG_FROM_PG_WAL;
+                else
+                    readFrom =3D currentSource =3D=3D XLOG_FROM_ARCHIVE ?
+                        XLOG_FROM_ANY : currentSource;

WALSourceSwitchState looks confusing here, and are you sure that this
is actualy correct?  Shouldn't we still try a READ_FROM_ANY or a read
=66rom the archives even with a streaming pending.

By the way, I am not convinced that what you have is the best
interface ever.  This assumes that we'd always want to switch to
streaming more aggressively.  Could there be a point in also
controlling if we should switch to pg_wal/ or just to archiving more
aggressively as well, aka be able to do the opposite switch of WAL
source?  This design looks somewhat limited to me.  The origin of the
issue is that we don't have a way to control the order of the sources
consumed by WAL replay.  Perhaps something like a replay_source_order
that uses a list would be better, with elements settable to archive,
pg_wal and streaming?
--
Michael

--eq3/hf7y7cv/yunH
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmX32kIACgkQnvQgOdby
QH1H1g/9HaNriy8iR/MA9sB6CFF38wI+NBZFeju0lzFI+OdiL3C2yd1+qn9WYNPq
YGbsxTZw6km6FE8qEN3+Cdtq1n9z7yKQBuoZ8MTwgl1qxyNr3Zi/OeSQGzssCP0C
xemSy8eRh0hGnfi1lpmj9c32i3a5hhYCvS6MXGEP3kEuwQULhl2Xh/wvrO0kl1ew
ePR9mfC6zjFmHFStXTedZR6jEZKZu4gJa7bCQdwbKNqQJ0BAqbpztDH3aM+5RjyV
GPr+8KL9N/Z8mKIZzqg8Jm+Nxv52P9OcBEhuzcJkOMFT3hW41v/ZPXaj9oh/8r9R
4G79IjRIwAPhJ0mAJ7GgvJZbbD5jqRuOeLOd/V3rdeI5mSlnS1vUIRw2PoFWLJcH
MfD7B2Mtn7TK7lpqwAkT86Lqwe4FF6+ludr+2q7L5NKpUKtzxkAA+PzRv7PY+eHc
36YERbMMV60UB0XLTD/xxLYWwY0OWk+6ytiCr2ac9/BLSxYSEGIUusvm+Hm8Ki+U
ED9AiNRcQtFVw/sqVjfIspGcpupJ8er1k9lkIyfrUuz5Tn1YLlXl6HONzBBXGL7X
5vUUYMBfF4tlNMmuXxXAu3hJwe7o5VCQJIOx8pj/+p2yYOLdLHzvj0GVh34O9aTX
nx7GsnYiaxoP1HmgN+Z74pD7OEN6RHp3qD4TzMokhR4IfUQ2tMA=
=t2D7
-----END PGP SIGNATURE-----

--eq3/hf7y7cv/yunH--