Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 2 Sep 2024 09:24:55 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Peter Eisentraut <peter@eisentraut.org>,
	Alexander Korotkov <akorotkov@postgresql.org>,
	pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Implement pg_wal_replay_wait() stored procedure
Message-ID: <ZtUF17gF0pNpwZDI@paquier.xyz>
References: <E1sZwuz-002NPQ-Lc@gemulon.postgresql.org>
 <18c0fa64-0475-415e-a1bd-665d922c5201@eisentraut.org>
 <CAPpHfduQPYWATA2rfbYXsXM+Yspn1nQVwrEs=p303O428J57Yg@mail.gmail.com>
 <ZtT4dhlxuxdPPI8t@paquier.xyz>
 <CAPpHfdubNR746cbxRvs8C0MmNvUTtrjWwZ=RzcB5i_4kUz+RuQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="HFMY63oezjKouoNS"
Content-Disposition: inline
In-Reply-To: 
 <CAPpHfdubNR746cbxRvs8C0MmNvUTtrjWwZ=RzcB5i_4kUz+RuQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZtUF17gF0pNpwZDI%40paquier.xyz>
Precedence: bulk


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

On Mon, Sep 02, 2024 at 02:55:50AM +0300, Alexander Korotkov wrote:
> Could you, please, check the attached patch?

The patch moving the code looks correct at quick glance.

Now, I've been staring at this line, wondering why this is required
while WaitForLSNReplay() does not return any status:
+   (void) WaitForLSNReplay(target_lsn, timeout);

And this makes me question whether you have the right semantics
regarding the SQL function itself.  Could it be more useful for
WaitForLSNReplay() to report an enum state that tells you the reason
why a wait may not have happened with a text or a tuple returned by
the function?  There are quite a few reasons why a wait may or may not
happen:=20
- Recovery has ended, target LSN has been reached.
- We're not in recovery anymore.  Is an error the most useful thing
here actually?
- Timeout may have been reached, where an error is also generated.
ERRCODE_QUERY_CANCELED is not a correct error state.
- Perhaps more of these in the future?

My point is: this is a feature that can be used for monitoring as far
as I know, still it does not stick as a feature that could be most
useful for such applications.  Wouldn't it be more useful for client
applications to deal with a state returned by the SQL function rather
than having to parse error strings to know what happened?  What is
returned by pg_wal_replay_wal() reflects on the design of
WaitForLSNReplay() itself.
--
Michael

--HFMY63oezjKouoNS
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmbVBdcACgkQnvQgOdby
QH2huhAAo5k/j3M2Gu10rAl0gq8Y/U3njx48/bW3zdHsSQG4tyz7BkR3ciTPpVii
y0e3C77MxK4mTZBhd880mMNP2TNkU1V3WBW3mD3Ytpaw2eIh2gQdgGwu8HSoCNKt
6hSot4+a22DLBPeoHiazrpg1FRedalTRdQPGT5eomysAiP4sMxh2edx3uF2FUZNd
e1d2H+VtUS6NznXoTccnva4Q9b+oLazrASFWmuWJtLSboYFWuNnJSfOb+k9oTEWv
QXMP/MZn+J0v3+QlD6PIfxTQNwkiKi+edAbejLLeB/AcXrDEfo7CUalgMacl44cI
kaeeTZmuXg2wGMOcBenhu/o7PHG47wlME1/nyk0+UEn27SAGSxqpsErkhn0dM5Ro
juP4qJmTWdSBnhZE6Y5DrA3wfL6bRof4OsIFNqLzI/Fqw0cnLIEnWgIgmjSECfDj
RtLOTqbGocT19pnrj96WijdhhaLM0AOCds5xGZE89sO1kJE9Cmaov/SD5JH0sBrj
iVJvr3eKGUIYWFiL5tqiguwdsAyOaEihXBGjU3m47w/kdgf2VDjGmKJLs1uq20/X
l6DVVBg5OcwH4GFrgZ5mmHirvmuTwiLom+0lY+/oqNt9kIJwIsAp6FYf7+PqK4AP
PUiyxw2BY6ideKCxGjwGBMntsFXgmaKzLpzBsaG36UWLg4dd9eE=
=nYhr
-----END PGP SIGNATURE-----

--HFMY63oezjKouoNS--