Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: Implement waiting for wal lsn replay: reloaded
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CABPTF7V0xMVS=U-pdFADDe6WEoGJ7jWZt7pffXfnmePfLWjZjg@mail.gmail.com>
Date: Fri, 26 Dec 2025 16:24:39 +0800
Cc: Alexander Korotkov <aekorotkov@gmail.com>,
 pgsql-hackers <pgsql-hackers@lists.postgresql.org>,
 Andres Freund <andres@anarazel.de>,
 =?utf-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>,
 Michael Paquier <michael@paquier.xyz>,
 jian he <jian.universality@gmail.com>,
 Tomas Vondra <tomas@vondra.me>,
 Yura Sokolov <y.sokolov@postgrespro.ru>
Content-Transfer-Encoding: quoted-printable
Message-Id: <B92C7AE6-FE91-46DB-9CBC-8795DBCBE217@gmail.com>
References: 
 <CAPpHfdsAsahNw7LQ1Yb6wN04RV2GGQ9XFiHnP_BDGZF=Yiickg@mail.gmail.com>
 <202511031242.5avrewkjqxyp@alvherre.pgsql>
 <m2suuaorc5lavczouk3nrt4t6y5mzrprr67ah4fr65gihv6eol@mmi4u6bg2hch>
 <CAPpHfdtK8NRxo1-u6kh1DjRGpcngBnHr5bahiY_5utWAbOjBDQ@mail.gmail.com>
 <CABPTF7Wd=JEPX560J3V=6pnT5bB6kQiHq5DPH=zQzG2dM6zQhg@mail.gmail.com>
 <CAPpHfdt4b0wBC4+Oopp_eFQnNjDvxwQLrQ1r4GMJfCY0XWP0dA@mail.gmail.com>
 <CABPTF7XiZuVORYz+EgzsKqyBo-QBsR73D5RTL7AK+0Go3tsscg@mail.gmail.com>
 <CABPTF7UiArgW-sXj9CNwRzUhYOQrevLzkYcgBydmX5oDes1sjg@mail.gmail.com>
 <CABPTF7W5mZ4MOBdfU6ftOXdX7mYR=yY-PNTEnvrObz4JRLccnQ@mail.gmail.com>
 <CABPTF7UJb=Buj6PHdgZLeZp0pYF4EtufkRpkeHF6XQDxcx=uSQ@mail.gmail.com>
 <CABPTF7VZgB0O1eYd-6BmYGw2a6qQN1XTts0a2VshSG5xMRObPQ@mail.gmail.com>
 <CABPTF7U2cYN=bMZirqj93Zv-aqBdw4f=wPRwovTzWKP=adYhDg@mail.gmail.com>
 <CABPTF7XbW0gSFMdhHq4suPhSaSTAHCHUp8twG4dwXHTGqQtNxA@mail.gmail.com>
 <CABPTF7XKti620ZAOXPGuhSZxCKyaV_9stq7ruhnuxvshUxCeRQ@mail.gmail.com>
 <CAPpHfdu_txw9UQ+wWYaHZoHTwt8sONUCoX2MPMakt+5iNh6wmQ@mail.gmail.com>
 <CABPTF7UYWvKiUuSoFT_so6OgiECGf_W5mMK7xQOZ_VYCNeyFyQ@mail.gmail.com>
 <CAPpHfdvLX6rhitxEsdxEDvO1fg5rOPH5G_wCodkFj6HKePfBcw@mail.gmail.com>
 <CABPTF7V0xMVS=U-pdFADDe6WEoGJ7jWZt7pffXfnmePfLWjZjg@mail.gmail.com>
To: Xuneng Zhou <xunengzhou@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/B92C7AE6-FE91-46DB-9CBC-8795DBCBE217%40gmail.com>
Precedence: bulk


> On Dec 19, 2025, at 10:49, Xuneng Zhou <xunengzhou@gmail.com> wrote:
>=20
> Hi,
>=20
> On Thu, Dec 18, 2025 at 8:25=E2=80=AFPM Alexander Korotkov =
<aekorotkov@gmail.com> wrote:
>>=20
>> On Thu, Dec 18, 2025 at 2:24=E2=80=AFPM Xuneng Zhou =
<xunengzhou@gmail.com> wrote:
>>> On Thu, Dec 18, 2025 at 6:38=E2=80=AFPM Alexander Korotkov =
<aekorotkov@gmail.com> wrote:
>>>>=20
>>>> Hi, Xuneng!
>>>>=20
>>>> On Tue, Dec 16, 2025 at 6:46=E2=80=AFAM Xuneng Zhou =
<xunengzhou@gmail.com> wrote:
>>>>> Remove the erroneous WAIT_LSN_TYPE_COUNT case from the switch
>>>>> statement in v5 patch 1.
>>>>=20
>>>> Thank you for your work on this patchset.  Generally, it looks like
>>>> good and quite straightforward extension of the current =
functionality.
>>>> But this patch adds 4 new unreserved keywords to our grammar.  Do =
you
>>>> think we can put mode into with options clause?
>>>>=20
>>>=20
>>> Thanks for pointing this out. Yeah, 4 unreserved keywords add
>>> complexity to the parser and it may not be worthwhile since replay =
is
>>> expected to be the common use scenario. Maybe we can do something =
like
>>> this:
>>>=20
>>> -- Default (REPLAY mode)
>>> WAIT FOR LSN '0/306EE20' WITH (TIMEOUT '1s');
>>>=20
>>> -- Explicit REPLAY mode
>>> WAIT FOR LSN '0/306EE20' WITH (MODE 'replay', TIMEOUT '1s');
>>>=20
>>> -- WRITE mode
>>> WAIT FOR LSN '0/306EE20' WITH (MODE 'write', TIMEOUT '1s');
>>>=20
>>> If no mode is set explicitly in the options clause, it defaults to
>>> replay. I'll update the patch per your suggestion.
>>=20
>> This is exactly what I meant.  Please, go ahead.
>>=20
>=20
> Here is the updated patch set (v7). Please check.
>=20
> --=20
> Best,
> Xuneng
> =
<v7-0001-Extend-xlogwait-infrastructure-with-write-and-flu.patch><v7-0004-=
Use-WAIT-FOR-LSN-in.patch><v7-0003-Add-tab-completion-for-WAIT-FOR-LSN-MOD=
E-option.patch><v7-0002-Add-MODE-option-to-WAIT-FOR-LSN-command.patch>

Hi Xuneng,

A solid patch! Just a few small comments:

1 - 0001
```
+XLogRecPtr
+GetCurrentLSNForWaitType(WaitLSNType lsnType)
+{
+	switch (lsnType)
+	{
+		case WAIT_LSN_TYPE_STANDBY_REPLAY:
+			return GetXLogReplayRecPtr(NULL);
+
+		case WAIT_LSN_TYPE_STANDBY_WRITE:
+			return GetWalRcvWriteRecPtr();
+
+		case WAIT_LSN_TYPE_STANDBY_FLUSH:
+			return GetWalRcvFlushRecPtr(NULL, NULL);
+
+		case WAIT_LSN_TYPE_PRIMARY_FLUSH:
+			return GetFlushRecPtr(NULL);
+	}
+
+	elog(ERROR, "invalid LSN wait type: %d", lsnType);
+	pg_unreachable();
+}
```

As you add pg_unreachable() in the new function =
GetCurrentLSNForWaitType(), I=E2=80=99m thinking if we should just do an =
Assert(), I saw every existing related function has done such an assert, =
for example addLSNWaiter(), it does =E2=80=9CAssert(i >=3D 0 && i < =
WAIT_LSN_TYPE_COUNT);=E2=80=9D. I guess we can just following the =
current mechanism to verify lsnType. So, for GetCurrentLSNForWaitType(), =
we can just add a default clause and Assert(false).

2 - 0002
```
+			else
+				ereport(ERROR,
+						=
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						 errmsg("unrecognized =
value for WAIT option \"%s\": \"%s\"",
+								"MODE", =
mode_str),
```

I wonder why don=E2=80=99t we directly put MODE into the error message?

3 - 0002
```
 		case WAIT_LSN_RESULT_NOT_IN_RECOVERY:
 			if (throw)
 			{
+				const		WaitLSNTypeDesc *desc =3D =
&WaitLSNTypeDescs[lsnType];
+				XLogRecPtr	currentLSN =3D =
GetCurrentLSNForWaitType(lsnType);
+
 				if (PromoteIsTriggered())
 				{
 					ereport(ERROR,
 							=
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 							errmsg("recovery =
is not in progress"),
-							=
errdetail("Recovery ended before replaying target LSN %X/%08X; last =
replay LSN %X/%08X.",
+							=
errdetail("Recovery ended before target LSN %X/%08X was %s; last %s LSN =
%X/%08X.",
 									 =
 LSN_FORMAT_ARGS(lsn),
-									 =
 LSN_FORMAT_ARGS(GetXLogReplayRecPtr(NULL))));
+									 =
 desc->verb,
+									 =
 desc->noun,
+									 =
 LSN_FORMAT_ARGS(currentLSN)));
 				}
 				else
 					ereport(ERROR,
 							=
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 							errmsg("recovery =
is not in progress"),
-							errhint("Waiting =
for the replay LSN can only be executed during recovery."));
+							errhint("Waiting =
for the %s LSN can only be executed during recovery.",
+									=
desc->noun));
 			}
```

currentLSN is only used in the if clause, thus it can be defined inside =
the if clause.

3 - 0002
```
+	/*
+	 * If we wrote an LSN that someone was waiting for then walk =
over the
+	 * shared memory array and set latches to notify the waiters.
+	 */
+	if (waitLSNState &&
+		(LogstreamResult.Write >=3D
+		 =
pg_atomic_read_u64(&waitLSNState->minWaitedLSN[WAIT_LSN_TYPE_STANDBY_WRITE=
])))
+		WaitLSNWakeup(WAIT_LSN_TYPE_STANDBY_WRITE, =
LogstreamResult.Write);
```

Do we need to mention "walk over the shared memory array and set =
latches=E2=80=9D in the comment? The logic belongs to WaitLSNWakeup(). =
What about if the wake up logic changes in future, then this comment =
would become stale. So I think we only need to mention =E2=80=9Cnotify =
the waiters=E2=80=9D.


4 - 0003
```
+	/*
+	 * Handle parenthesized option list.  This fires when we're in =
an
+	 * unfinished parenthesized option list.  get_previous_words =
treats a
+	 * completed parenthesized option list as one word, so the above =
test is
+	 * correct.  mode takes a string value ('replay', 'write', =
'flush'),
+	 * timeout takes a string value, no_throw takes no value.
+	 */
 	else if (HeadMatches("WAIT", "FOR", "LSN", MatchAny, "WITH", =
"(*") &&
 			 !HeadMatches("WAIT", "FOR", "LSN", MatchAny, =
"WITH", "(*)"))
 	{
-		/*
-		 * This fires if we're in an unfinished parenthesized =
option list.
-		 * get_previous_words treats a completed parenthesized =
option list as
-		 * one word, so the above test is correct.
-		 */
 		if (ends_with(prev_wd, '(') || ends_with(prev_wd, ','))
-			COMPLETE_WITH("timeout", "no_throw");
-
-		/*
-		 * timeout takes a string value, no_throw takes no =
value. We don't
-		 * offer completions for these values.
-		 */
```

The new comment has lost the meaning of =E2=80=9CWe don=E2=80=99t offer =
completions for these values (timeout and no_throw)=E2=80=9D, to be =
explicit, I feel we can retain the sentence.

5 - 0004
```
+	my $isrecovery =3D
+	  $self->safe_psql('postgres', "SELECT pg_is_in_recovery()");
+	chomp($isrecovery);
 	croak "unknown mode $mode for 'wait_for_catchup', valid modes =
are "
 	  . join(', ', keys(%valid_modes))
 	  unless exists($valid_modes{$mode});
@@ -3347,9 +3350,6 @@ sub wait_for_catchup
 	}
 	if (!defined($target_lsn))
 	{
-		my $isrecovery =3D
-		  $self->safe_psql('postgres', "SELECT =
pg_is_in_recovery()");
-		chomp($isrecovery);
```

I wonder why pull up pg_is_in_recovery to an early place and =
unconditionally call it?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/