> On Dec 19, 2025, at 10:49, Xuneng Zhou <[email protected]> wrote:
>
> Hi,
>
> On Thu, Dec 18, 2025 at 8:25 PM Alexander Korotkov <[email protected]>
> wrote:
>>
>> On Thu, Dec 18, 2025 at 2:24 PM Xuneng Zhou <[email protected]> wrote:
>>> On Thu, Dec 18, 2025 at 6:38 PM Alexander Korotkov <[email protected]>
>>> wrote:
>>>>
>>>> Hi, Xuneng!
>>>>
>>>> On Tue, Dec 16, 2025 at 6:46 AM Xuneng Zhou <[email protected]> wrote:
>>>>> Remove the erroneous WAIT_LSN_TYPE_COUNT case from the switch
>>>>> statement in v5 patch 1.
>>>>
>>>> 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?
>>>>
>>>
>>> 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:
>>>
>>> -- Default (REPLAY mode)
>>> WAIT FOR LSN '0/306EE20' WITH (TIMEOUT '1s');
>>>
>>> -- Explicit REPLAY mode
>>> WAIT FOR LSN '0/306EE20' WITH (MODE 'replay', TIMEOUT '1s');
>>>
>>> -- WRITE mode
>>> WAIT FOR LSN '0/306EE20' WITH (MODE 'write', TIMEOUT '1s');
>>>
>>> If no mode is set explicitly in the options clause, it defaults to
>>> replay. I'll update the patch per your suggestion.
>>
>> This is exactly what I meant. Please, go ahead.
>>
>
> Here is the updated patch set (v7). Please check.
>
> --
> 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-MODE-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’m
thinking if we should just do an Assert(), I saw every existing related
function has done such an assert, for example addLSNWaiter(), it does “Assert(i
>= 0 && i < WAIT_LSN_TYPE_COUNT);”. 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’t we directly put MODE into the error message?
3 - 0002
```
case WAIT_LSN_RESULT_NOT_IN_RECOVERY:
if (throw)
{
+ const WaitLSNTypeDesc *desc =
&WaitLSNTypeDescs[lsnType];
+ XLogRecPtr currentLSN =
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 >=
+
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” 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 “notify the waiters”.
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 “We don’t offer completions for these
values (timeout and no_throw)”, to be explicit, I feel we can retain the
sentence.
5 - 0004
```
+ my $isrecovery =
+ $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 =
- $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/
