Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 27 Sep 2024 13:35:23 +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: <ZvY2C8N4ZqgCFaLu@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>
 <ZtUF17gF0pNpwZDI@paquier.xyz>
 <CAPpHfduN68AzUHvzzPG80qwa-27QXjd820tzcjoUk2Tc6_O=5A@mail.gmail.com>
 <CAPpHfdv2kE8SqT54QChJtieewnpOuF9qvTJ+F_8LPUCEHrM=JQ@mail.gmail.com>
 <CAPpHfdsw9oq62Fvt65JApHJf1auUirdGJV7=nRyVnDL3M8z5xA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Ux2IJytT1P5dprhM"
Content-Disposition: inline
In-Reply-To: 
 <CAPpHfdsw9oq62Fvt65JApHJf1auUirdGJV7=nRyVnDL3M8z5xA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZvY2C8N4ZqgCFaLu%40paquier.xyz>
Precedence: bulk


--Ux2IJytT1P5dprhM
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, Sep 20, 2024 at 03:00:20PM +0300, Alexander Korotkov wrote:
> Please, check the attached patchset for implementation of proposed approach.

0001 looks like it requires an indentation in its .h diffs.

+typedef enum
+{
+	WaitLSNResultSuccess, /* Target LSN is reached */
+	WaitLSNResultTimeout, /* Timeout occured */

Perhaps use WAIT_LSN_RESULT_SUCCESS, etc, rather than camel casing.

+ * Results statuses for WaitForLSNReplay().

Too much plural here.

What you are doing with WaitForLSNReplay() sounds kind of right.

        rc = WaitLatch(MyLatch, wake_events, delay_ms,
             WAIT_EVENT_WAIT_FOR_WAL_REPLAY);

Question about this existing bit in waitlsn.c.  Shouldn't this issue a
FATAL if rc reports a WL_POSTMASTER_DEATH?  Or am I missing an
intention here.  That was already the case before this patch set.

pg_wal_replay_wait() is new to v18, meaning that we still have some
time to agree on its final shape for this release cycle.  This
discussion shares similarities with the recent exercise done in
f593c5517d14, and I think that we should be more consistent between
both to not repeat the same mistakes as in the past, even if this case
if more complex because we have more reasons behind why a wait could
not have happened.

I would suggest to keep things simple and have one single function
rather than introduce two more pg_proc entries with slight differences
in their error reporting, making the original function return a text
about the reason of the failure when waiting (be it a timeout,
success, promotion or outside recovery).

FWIW, I am confused regarding the need for WaitLSNResultNotInRecovery
and WaitLSNResultPromotedConcurrently in the error states.  On a code
basis, they check the same thing: RecoveryInProgress().  I'd suggest
to cut one of them.  This also points to the fact that
WaitForLSNReplay() has some duplication that's not required.  We could
have less GetXLogReplayRecPtr() calls and do all the checks in the for
loop.  The current structure of the code in waitlsn.c is more complex
than it could be.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmb2NgsACgkQnvQgOdby
QH3rMRAAnUEvRU4xb7aKR56yLNC77updeBGe/avsn5cjqHdj0UDSwhEyZhdEjpAt
9N0QvCEsHLrYIcnnkSi4B9kEV6IwNZdTsAEmQzTeaOoYAr9AmM7UfYtK1wAhd53N
KAHmoxE50DBA0pUa9vwCutTsu3pxRXeeeXBmf4WuWo6dRTXRRzSWNPofeTjoiTaM
hQcMK21g6v7egOUI2ouc+5GlrBRlQ/dkUwlyx5gbqKUXov/MINtNKwXDIUQlUptH
wA/42ZC1HN9IQqOv2mNMtHTcdvwL9NsFkZ6Y4xQcU1qUorC9KiAlUr2yyz3fuTwK
28JPcgPgEM6urZ8MTg7nHRNrRswSLla0GDmjVgvtkOYr00OOYQrg6Cwz5I/IOGKx
cuyKr6g1E/S3PIb5W2PXY3EobGE3Lop3rAnJlL7cM2QF1A0D+CIF964ydZBIoqo4
9NQnVybLCJ75gS5rNlom/3ScV/N2Sw91tOUd44cdIJlnfY/Z4UAuktbhrl55rJcl
uSDMcwUI96j8NgGEzm5KQaEYA2V+04wL92nVdngvn29g2WiE7x3f6Q5pnLI9WkaF
PkIdtNQqk9hhIaTSAtk02VXSIyuMzv0B6I5ogaPKXqMck2n8Nc6vVtXCA8Y2Lb1Z
DflSf4Ycue8lX0Wfc63XA7wjNrKpyjNYqv0zKEmAN2b4XoVGQa4=
=YaqN
-----END PGP SIGNATURE-----

--Ux2IJytT1P5dprhM--