Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Clean up 019_replslot_limit.pl comments

  2. Stabilize 019_replslot_limit.pl: wait on slot restart_lsn

  3. Use WAIT FOR LSN in PostgreSQL::Test::Cluster::wait_for_catchup()

  4. Add tab completion for the WAIT FOR LSN MODE option

  5. Add the MODE option to the WAIT FOR LSN command

  6. Extend xlogwait infrastructure with write and flush wait types

  7. Unify error messages

  8. Optimize shared memory usage for WaitLSNProcInfo

  9. Fix WaitLSNWakeup() fast-path check for InvalidXLogRecPtr

  10. Fix incorrect function name in comments

  11. Add infrastructure for efficient LSN waiting

  12. Add pairingheap_initialize() for shared memory usage

  13. Implement WAIT FOR command

  1. Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2024-11-27T04:08:51Z

    Hi!

Introduction

The simple way to wait for a given lsn to replay on standby appears to
be useful because it provides a way to achieve read-your-writes
consistency while working with both replication leader and standby.
And it's both handy and cheaper to have built-in functionality for
that instead of polling pg_last_wal_replay_lsn().

Key problem

While this feature generally looks trivial, there is a surprisingly
hard problem.  While waiting for an LSN to replay, you should hold any
snapshots.  If you hold a snapshot on standby, that snapshot could
prevent the replay of WAL records.  In turn, that could prevent the
wait to finish, causing a kind of deadlock.  Therefore, waiting for
LSN to replay couldn't be implemented as a function.  My last attempt
implements this functionality as a stored procedure [1].  This
approach generally works but has a couple of serious limitations.
1) Given that a CALL statement has to lookup a catalog for the stored
procedure, we can't work inside a transaction of REPEATABLE READ or a
higher isolation level (even if nothing has been done before in that
transaction).  It is especially unpleasant that this limitation covers
the case of the implicit transaction when
default_transaction_isolation = 'repeatable read' [2].  I had a
workaround for that [3], but it looks a bit awkward.
2) Using output parameters for a stored procedure causes an extra
snapshot to be held.  And that snapshot is difficult (unsafe?) to
release [3].

Present solution

The present patch implements a new utility command WAIT FOR LSN
'target_lsn' [, TIMEOUT 'timeout'][, THROW 'throw'].  Unlike previous
attempts to implement custom syntax, it uses only one extra unreserved
keyword.  The parameters are implemented as generic_option_list.

Custom syntax eliminates the problem of running within an empty
transaction of REPEATABLE READ level or higher.  We don't need to
lookup a system catalog.  Thus, we have to set a transaction snapshot.

Also, revising PlannedStmtRequiresSnapshot() allows us to avoid
holding a snapshot to return a value.  Therefore, the WAIT command in
the attached patch returns its result status.

Also, the attached patch explicitly checks if the standby has been
promoted to throw the most relevant form of an error.  The issue of
inaccurate error messages has been previously spotted in [5].

Any comments?

Links.
1. https://www.postgresql.org/message-id/E1sZwuz-002NPQ-Lc%40gemulon.postgresql.org
2. https://www.postgresql.org/message-id/14de8671-e328-4c3e-b136-664f6f13a39f%40iki.fi
3. https://www.postgresql.org/message-id/CAPpHfdvRmTzGJw5rQdSMkTxUPZkjwtbQ%3DLJE2u9Jqh9gFXHpmg%40mail.gmail.com
4. https://www.postgresql.org/message-id/4953563546cb8c8851f84c7debf723ef%40postgrespro.ru
5. https://www.postgresql.org/message-id/ab0eddce-06d4-4db2-87ce-46fa2427806c%40iki.fi

------
Regards,
Alexander Korotkov
Supabase

  2. Re: Implement waiting for wal lsn replay: reloaded

    Kirill Reshke <reshkekirill@gmail.com> — 2024-12-04T11:12:07Z

    On Wed, 27 Nov 2024 at 09:09, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> Hi!
>
> Introduction
>
> The simple way to wait for a given lsn to replay on standby appears to
> be useful because it provides a way to achieve read-your-writes
> consistency while working with both replication leader and standby.
> And it's both handy and cheaper to have built-in functionality for
> that instead of polling pg_last_wal_replay_lsn().
>
> Key problem
>
> While this feature generally looks trivial, there is a surprisingly
> hard problem.  While waiting for an LSN to replay, you should hold any
> snapshots.  If you hold a snapshot on standby, that snapshot could
> prevent the replay of WAL records.  In turn, that could prevent the
> wait to finish, causing a kind of deadlock.  Therefore, waiting for
> LSN to replay couldn't be implemented as a function.  My last attempt
> implements this functionality as a stored procedure [1].  This
> approach generally works but has a couple of serious limitations.
> 1) Given that a CALL statement has to lookup a catalog for the stored
> procedure, we can't work inside a transaction of REPEATABLE READ or a
> higher isolation level (even if nothing has been done before in that
> transaction).  It is especially unpleasant that this limitation covers
> the case of the implicit transaction when
> default_transaction_isolation = 'repeatable read' [2].  I had a
> workaround for that [3], but it looks a bit awkward.
> 2) Using output parameters for a stored procedure causes an extra
> snapshot to be held.  And that snapshot is difficult (unsafe?) to
> release [3].
>
> Present solution
>
> The present patch implements a new utility command WAIT FOR LSN
> 'target_lsn' [, TIMEOUT 'timeout'][, THROW 'throw'].  Unlike previous
> attempts to implement custom syntax, it uses only one extra unreserved
> keyword.  The parameters are implemented as generic_option_list.
>
> Custom syntax eliminates the problem of running within an empty
> transaction of REPEATABLE READ level or higher.  We don't need to
> lookup a system catalog.  Thus, we have to set a transaction snapshot.
>
> Also, revising PlannedStmtRequiresSnapshot() allows us to avoid
> holding a snapshot to return a value.  Therefore, the WAIT command in
> the attached patch returns its result status.
>
> Also, the attached patch explicitly checks if the standby has been
> promoted to throw the most relevant form of an error.  The issue of
> inaccurate error messages has been previously spotted in [5].
>
> Any comments?
>
> Links.
> 1. https://www.postgresql.org/message-id/E1sZwuz-002NPQ-Lc%40gemulon.postgresql.org
> 2. https://www.postgresql.org/message-id/14de8671-e328-4c3e-b136-664f6f13a39f%40iki.fi
> 3. https://www.postgresql.org/message-id/CAPpHfdvRmTzGJw5rQdSMkTxUPZkjwtbQ%3DLJE2u9Jqh9gFXHpmg%40mail.gmail.com
> 4. https://www.postgresql.org/message-id/4953563546cb8c8851f84c7debf723ef%40postgrespro.ru
> 5. https://www.postgresql.org/message-id/ab0eddce-06d4-4db2-87ce-46fa2427806c%40iki.fi
>
> ------
> Regards,
> Alexander Korotkov
> Supabase
Hi!

What's the current status of
https://commitfest.postgresql.org/50/5167/ ? Should we close it or
reattach to this thread?


-- 
Best regards,
Kirill Reshke

  3. Re: Implement waiting for wal lsn replay: reloaded

    Andrei Lepikhov <lepihov@gmail.com> — 2025-02-06T07:42:12Z

    On 12/4/24 18:12, Kirill Reshke wrote:
> On Wed, 27 Nov 2024 at 09:09, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>> Any comments?
> What's the current status of
> https://commitfest.postgresql.org/50/5167/ ? Should we close it or
> reattach to this thread?
To push this feature further I rebased the patch onto current master.
Also, let's add a commitfest entry:
https://commitfest.postgresql.org/52/5550/

-- 
regards, Andrei Lepikhov

  4. Re: Implement waiting for wal lsn replay: reloaded

    Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-02-06T08:31:28Z

    27.11.2024 07:08, Alexander Korotkov wrote:
> Present solution
> 
> The present patch implements a new utility command WAIT FOR LSN
> 'target_lsn' [, TIMEOUT 'timeout'][, THROW 'throw'].  Unlike previous
> attempts to implement custom syntax, it uses only one extra unreserved
> keyword.  The parameters are implemented as generic_option_list.
> 
> Custom syntax eliminates the problem of running within an empty
> transaction of REPEATABLE READ level or higher.  We don't need to
> lookup a system catalog.  Thus, we have to set a transaction snapshot.
> 
> Also, revising PlannedStmtRequiresSnapshot() allows us to avoid
> holding a snapshot to return a value.  Therefore, the WAIT command in
> the attached patch returns its result status.
> 
> Also, the attached patch explicitly checks if the standby has been
> promoted to throw the most relevant form of an error.  The issue of
> inaccurate error messages has been previously spotted in [5].
> 
> Any comments?

Good day, Alexander.

I briefly looked into patch and have couple of minor remarks:

1. I don't like `palloc` in the `WaitLSNWakeup`. I believe it wont issue
problems, but still don't like it. I'd prefer to see local fixed array, say
of 16 elements, and loop around remaining function body acting in batch of
16 wakeups. Doubtfully there will be more than 16 waiting clients often,
and even then it wont be much heavier than fetching all at once.

2. I'd move `inHeap` field between `procno` and `phNode` to fill the gap
between fields on 64bit platforms.
Well, I believe, it would be better to tweak `pairingheap_node` to make it
clear if it is in heap or not. But such change would be unrelated to
current patch's sense. So lets stick with `inHeap`, but move it a bit.

Non-code question: do you imagine for `WAIT` command reuse for other cases?
Is syntax rule in gram.y convenient enough for such reuse? I believe, `LSN`
is not part of syntax to not introduce new keyword. But is it correct way?
I have no answer or strong opinion.

Otherwise, the patch looks quite strong to me.

-------
regards
Yura Sokolov

  5. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-02-16T21:27:43Z

    Hi, Yura!


On Thu, Feb 6, 2025 at 10:31 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote:
> I briefly looked into patch and have couple of minor remarks:
>
> 1. I don't like `palloc` in the `WaitLSNWakeup`. I believe it wont issue
> problems, but still don't like it. I'd prefer to see local fixed array, say
> of 16 elements, and loop around remaining function body acting in batch of
> 16 wakeups. Doubtfully there will be more than 16 waiting clients often,
> and even then it wont be much heavier than fetching all at once.

OK, I've refactored this to use static array of 16 size.  palloc() is
used only if we don't fit static array.

> 2. I'd move `inHeap` field between `procno` and `phNode` to fill the gap
> between fields on 64bit platforms.
> Well, I believe, it would be better to tweak `pairingheap_node` to make it
> clear if it is in heap or not. But such change would be unrelated to
> current patch's sense. So lets stick with `inHeap`, but move it a bit.

Ok, `inHeap` is moved.

> Non-code question: do you imagine for `WAIT` command reuse for other cases?
> Is syntax rule in gram.y convenient enough for such reuse? I believe, `LSN`
> is not part of syntax to not introduce new keyword. But is it correct way?
> I have no answer or strong opinion.

This is conscious decision.  New rules and new keywords causes extra
states for parser state machine.  There could be raised a question
whether feature is valuable enough to justify the slowdown of parser.
This is why I tried to make this feature as less invasive as possible
in terms of parser.  And yes, there potentially could be other things
to wait.  For instance, instead of waiting for lsn replay we could be
waiting for finishing replay of given xid.

> Otherwise, the patch looks quite strong to me.

Great, thank you!

------
Regards,
Alexander Korotkov
Supabase

  6. Re: Implement waiting for wal lsn replay: reloaded

    Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-02-28T13:03:33Z

    17.02.2025 00:27, Alexander Korotkov wrote:
> On Thu, Feb 6, 2025 at 10:31 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote:
>> I briefly looked into patch and have couple of minor remarks:
>>
>> 1. I don't like `palloc` in the `WaitLSNWakeup`. I believe it wont issue
>> problems, but still don't like it. I'd prefer to see local fixed array, say
>> of 16 elements, and loop around remaining function body acting in batch of
>> 16 wakeups. Doubtfully there will be more than 16 waiting clients often,
>> and even then it wont be much heavier than fetching all at once.
> 
> OK, I've refactored this to use static array of 16 size.  palloc() is
> used only if we don't fit static array.

I've rebased patch and:
- fixed compiler warning in wait.c ("maybe uninitialized 'result'").
- made a loop without call to palloc in WaitLSNWakeup. It is with "goto" to
keep indentation, perhaps `do {} while` would be better?

-------
regards
Yura Sokolov aka funny-falcon

  7. Re: Implement waiting for wal lsn replay: reloaded

    Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-02-28T13:55:21Z

    28.02.2025 16:03, Yura Sokolov пишет:
> 17.02.2025 00:27, Alexander Korotkov wrote:
>> On Thu, Feb 6, 2025 at 10:31 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote:
>>> I briefly looked into patch and have couple of minor remarks:
>>>
>>> 1. I don't like `palloc` in the `WaitLSNWakeup`. I believe it wont issue
>>> problems, but still don't like it. I'd prefer to see local fixed array, say
>>> of 16 elements, and loop around remaining function body acting in batch of
>>> 16 wakeups. Doubtfully there will be more than 16 waiting clients often,
>>> and even then it wont be much heavier than fetching all at once.
>>
>> OK, I've refactored this to use static array of 16 size.  palloc() is
>> used only if we don't fit static array.
> 
> I've rebased patch and:
> - fixed compiler warning in wait.c ("maybe uninitialized 'result'").
> - made a loop without call to palloc in WaitLSNWakeup. It is with "goto" to
> keep indentation, perhaps `do {} while` would be better?

And fixed:
   'WAIT' is marked as BARE_LABEL in kwlist.h, but it is missing from
gram.y's bare_label_keyword rule

-------
regards
Yura Sokolov aka funny-falcon

  8. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-03-10T11:30:31Z

    On Fri, Feb 28, 2025 at 3:55 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote:
> 28.02.2025 16:03, Yura Sokolov пишет:
> > 17.02.2025 00:27, Alexander Korotkov wrote:
> >> On Thu, Feb 6, 2025 at 10:31 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote:
> >>> I briefly looked into patch and have couple of minor remarks:
> >>>
> >>> 1. I don't like `palloc` in the `WaitLSNWakeup`. I believe it wont issue
> >>> problems, but still don't like it. I'd prefer to see local fixed array, say
> >>> of 16 elements, and loop around remaining function body acting in batch of
> >>> 16 wakeups. Doubtfully there will be more than 16 waiting clients often,
> >>> and even then it wont be much heavier than fetching all at once.
> >>
> >> OK, I've refactored this to use static array of 16 size.  palloc() is
> >> used only if we don't fit static array.
> >
> > I've rebased patch and:
> > - fixed compiler warning in wait.c ("maybe uninitialized 'result'").
> > - made a loop without call to palloc in WaitLSNWakeup. It is with "goto" to
> > keep indentation, perhaps `do {} while` would be better?
>
> And fixed:
>    'WAIT' is marked as BARE_LABEL in kwlist.h, but it is missing from
> gram.y's bare_label_keyword rule

Thank you, Yura.  I've further revised the patch.  Mostly added the
documentation including SQL command reference and few paragraphs in
the high availability chapter explaining the read-your-writes
consistency concept.

------
Regards,
Alexander Korotkov
Supabase

  9. Re: Implement waiting for wal lsn replay: reloaded

    Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-03-12T14:44:28Z

    10.03.2025 14:30, Alexander Korotkov пишет:
> On Fri, Feb 28, 2025 at 3:55 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote:
>> 28.02.2025 16:03, Yura Sokolov пишет:
>>> 17.02.2025 00:27, Alexander Korotkov wrote:
>>>> On Thu, Feb 6, 2025 at 10:31 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote:
>>>>> I briefly looked into patch and have couple of minor remarks:
>>>>>
>>>>> 1. I don't like `palloc` in the `WaitLSNWakeup`. I believe it wont issue
>>>>> problems, but still don't like it. I'd prefer to see local fixed array, say
>>>>> of 16 elements, and loop around remaining function body acting in batch of
>>>>> 16 wakeups. Doubtfully there will be more than 16 waiting clients often,
>>>>> and even then it wont be much heavier than fetching all at once.
>>>>
>>>> OK, I've refactored this to use static array of 16 size.  palloc() is
>>>> used only if we don't fit static array.
>>>
>>> I've rebased patch and:
>>> - fixed compiler warning in wait.c ("maybe uninitialized 'result'").
>>> - made a loop without call to palloc in WaitLSNWakeup. It is with "goto" to
>>> keep indentation, perhaps `do {} while` would be better?
>>
>> And fixed:
>>    'WAIT' is marked as BARE_LABEL in kwlist.h, but it is missing from
>> gram.y's bare_label_keyword rule
> 
> Thank you, Yura.  I've further revised the patch.  Mostly added the
> documentation including SQL command reference and few paragraphs in
> the high availability chapter explaining the read-your-writes
> consistency concept.

Good day, Alexander.

Looking "for the last time" to the patch I found there remains
`pg_wal_replay_wait` function in documentation and one comment.
So I fixed it in documentation, and removed sentence from comment.

Otherwise v6 is just rebased v5.

-------
regards
Yura Sokolov aka funny-falcon

  10. Re: Implement waiting for wal lsn replay: reloaded

    Tomas Vondra <tomas@vondra.me> — 2025-03-13T14:15:01Z

    Hi,

I did a quick look at this patch. I haven't found any correctness
issues, but I have some general review comments and questions about the
grammar / syntax.

1) The sgml docs don't really show the syntax very nicely, it only shows
this at the beginning of wait_for.sgml:

   WAIT FOR ( <replaceable class="parameter">parameter</replaceable>
'<replaceable class="parameter">value</replaceable>' [, ... ] ) ]

I kinda understand this comes from using the generic option list (I'll
get to that shortly), but I think it'd be much better to actually show
the "full" syntax here, instead of leaving the "parameters" to later.


2) The syntax description suggests "(" and ")" are required, but that
does not seem to be the case - in fact, it's not even optional, and when
I try using that, I get syntax error.


3) I have my doubts about using the generic_option_list for this. Yes, I
understand this allows using fewer reserved keywords, but it leads to
some weirdness and I'm not sure it's worth it. Not sure what the right
trade off is here.

Anyway, some examples of the weird stuff implied by this approach:

- it forces "," between the options, which is a clear difference from
what we do for every other command

- it forces everything to be a string, i.e. you can' say "TIMEOUT 10",
it has to be "TIMEOUT '10'"

I don't have a very strong opinion on this, but the result seems a bit
strange to me.


4) I'm not sure I understand the motivation of the "throw false" mode,
and I'm not sure I understand this description in the sgml docs:

    On timeout, or if the server is promoted before
    <parameter>lsn</parameter> is reached, an error is emitted,
    as soon as <parameter>throw</parameter> is not specified or set to
    true.
    If <parameter>throw</parameter> is set to false, then the command
    doesn't throw errors.

I find it a bit confusing. What is the use case for this mode?


5) One place in the docs says:

      The target log sequence number to wait for.

   Thie is literally the only place using "log sequence number" in our
   code base, I'd just use "LSN" just like every other place.


6) The docs for the TIMEOUT parameter say this:

   <varlistentry>
    <term><replaceable class="parameter">timeout</replaceable></term>
    <listitem>
     <para>
      When specified and greater than zero, the command waits until
      <parameter>lsn</parameter> is reached or the specified
      <parameter>timeout</parameter> has elapsed.  Must be a non-
      negative integer, the default is zero.
     </para>
    </listitem>
   </varlistentry>

   That doesn't say what unit does the option use. Is is seconds,
   milliseconds or what?

   In fact, it'd be nice to let users specify that in the value, similar
   to other options (e.g. SET statement_timeout = '10s').


7) One place in the docs says this:

    That is, after this function execution, the value returned by
    <function>pg_last_wal_replay_lsn</function> should be greater ...

  I think the reference to "function execution" is obsolete?


8) I find this confusing:

    However, if <command>WAIT FOR</command> is
    called on primary promoted from standby and <literal>lsn</literal>
    was already replayed, then the <command>WAIT FOR</command> command
    just exits immediately.

  Does this mean running the WAIT command on a primary (after it was
  already promoted) will exit immediately? Why does it matter that it
  was promoted from a standby? Shouldn't it exit immediately even for
  a standalone instance?


9) xlogwait.c

I think this should start with a basic "design" description of how the
wait is implemented, in a comment at the top of the file. That is, what
we keep in the shared memory, what happens during a wait, how it uses
the pairing heap, etc. After reading this comment I should understand
how it all fits together.


10) WaitForLSNReplay / WaitLSNWakeup

I think the function comment should document the important stuff (e.g.
return values for various situations, how it groups waiters into chunks
of 16 elements during wakeup, ...).


11) WaitLSNProcInfo / WaitLSNState

Does this need to be exposed in xlogwait.h? These structs seem private
to xlogwait.c, so maybe declare it there?


regards

-- 
Tomas Vondra

  11. Re: Implement waiting for wal lsn replay: reloaded

    vignesh C <vignesh21@gmail.com> — 2025-03-16T13:32:11Z

    On Wed, 12 Mar 2025 at 20:14, Yura Sokolov <y.sokolov@postgrespro.ru> wrote:
>
> Otherwise v6 is just rebased v5.

I noticed that Tomas's comments from [1] are not yet addressed, I have
changed the commitfest status to Waiting on Author, please address
them and update it to Needs review.
[1] - https://www.postgresql.org/message-id/09a98dc9-eeb1-471d-b990-072513c3d584@vondra.me

Regards,
Vignesh

  12. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-04-29T11:27:25Z

    Hi, Tomas.

Thank you so much for your review!  Please find the revised patchset.

On Thu, Mar 13, 2025 at 4:15 PM Tomas Vondra <tomas@vondra.me> wrote:
> I did a quick look at this patch. I haven't found any correctness
> issues, but I have some general review comments and questions about the
> grammar / syntax.
>
> 1) The sgml docs don't really show the syntax very nicely, it only shows
> this at the beginning of wait_for.sgml:
>
>    WAIT FOR ( <replaceable class="parameter">parameter</replaceable>
> '<replaceable class="parameter">value</replaceable>' [, ... ] ) ]
>
> I kinda understand this comes from using the generic option list (I'll
> get to that shortly), but I think it'd be much better to actually show
> the "full" syntax here, instead of leaving the "parameters" to later.

Sounds reasonable, changed to show the full syntax in the synopsis.

> 2) The syntax description suggests "(" and ")" are required, but that
> does not seem to be the case - in fact, it's not even optional, and when
> I try using that, I get syntax error.

Good catch, fixed.

> 3) I have my doubts about using the generic_option_list for this. Yes, I
> understand this allows using fewer reserved keywords, but it leads to
> some weirdness and I'm not sure it's worth it. Not sure what the right
> trade off is here.
>
> Anyway, some examples of the weird stuff implied by this approach:
>
> - it forces "," between the options, which is a clear difference from
> what we do for every other command
>
> - it forces everything to be a string, i.e. you can' say "TIMEOUT 10",
> it has to be "TIMEOUT '10'"
>
> I don't have a very strong opinion on this, but the result seems a bit
> strange to me.

I've improved the syntax.  I still tried to keep the number of new
keywords and grammar rules minimal.  That leads to moving some parser
login into wait.c.  This is probably a bit awkward, but saves our
grammar from bloat.  Let me know what do you think about this
approach.

> 4) I'm not sure I understand the motivation of the "throw false" mode,
> and I'm not sure I understand this description in the sgml docs:
>
>     On timeout, or if the server is promoted before
>     <parameter>lsn</parameter> is reached, an error is emitted,
>     as soon as <parameter>throw</parameter> is not specified or set to
>     true.
>     If <parameter>throw</parameter> is set to false, then the command
>     doesn't throw errors.
>
> I find it a bit confusing. What is the use case for this mode?

The idea here is that application could do some handling of these
errors without having to parse the error messages (parsing error
messages is inconvenient because of localization etc).

> 5) One place in the docs says:
>
>       The target log sequence number to wait for.
>
>    Thie is literally the only place using "log sequence number" in our
>    code base, I'd just use "LSN" just like every other place.

OK fixed.

> 6) The docs for the TIMEOUT parameter say this:
>
>    <varlistentry>
>     <term><replaceable class="parameter">timeout</replaceable></term>
>     <listitem>
>      <para>
>       When specified and greater than zero, the command waits until
>       <parameter>lsn</parameter> is reached or the specified
>       <parameter>timeout</parameter> has elapsed.  Must be a non-
>       negative integer, the default is zero.
>      </para>
>     </listitem>
>    </varlistentry>
>
>    That doesn't say what unit does the option use. Is is seconds,
>    milliseconds or what?
>
>    In fact, it'd be nice to let users specify that in the value, similar
>    to other options (e.g. SET statement_timeout = '10s').

The default unit of milliseconds is specified.  Also, an alternative
way to specify timeout is now supported.  Timeout might be a string
literal consisting of numeric and unit specifier.

> 7) One place in the docs says this:
>
>     That is, after this function execution, the value returned by
>     <function>pg_last_wal_replay_lsn</function> should be greater ...
>
>   I think the reference to "function execution" is obsolete?

Actually, this is just the function, which reports current replay LSN,
not function introduced by previous version of this patch.  We refer
it to just express the constraint that LSN must be replayed after
execution of the command.

> 8) I find this confusing:
>
>     However, if <command>WAIT FOR</command> is
>     called on primary promoted from standby and <literal>lsn</literal>
>     was already replayed, then the <command>WAIT FOR</command> command
>     just exits immediately.
>
>   Does this mean running the WAIT command on a primary (after it was
>   already promoted) will exit immediately? Why does it matter that it
>   was promoted from a standby? Shouldn't it exit immediately even for
>   a standalone instance?

I think the previous sentence should give an idea that otherwise error
gets thrown.  That also happens immediately for sure.

> 9) xlogwait.c
>
> I think this should start with a basic "design" description of how the
> wait is implemented, in a comment at the top of the file. That is, what
> we keep in the shared memory, what happens during a wait, how it uses
> the pairing heap, etc. After reading this comment I should understand
> how it all fits together.

OK, I've added the header comment.

> 10) WaitForLSNReplay / WaitLSNWakeup
>
> I think the function comment should document the important stuff (e.g.
> return values for various situations, how it groups waiters into chunks
> of 16 elements during wakeup, ...).

Revised header comments for those functions too.

> 11) WaitLSNProcInfo / WaitLSNState
>
> Does this need to be exposed in xlogwait.h? These structs seem private
> to xlogwait.c, so maybe declare it there?

Hmm, I don't remember why I moved them to xlogwait.h.  OK, moved them
back to xlogwait.c.


------
Regards,
Alexander Korotkov
Supabase

  13. Re: Implement waiting for wal lsn replay: reloaded

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-08-05T13:47:07Z

    On 2025-Apr-29, Alexander Korotkov wrote:

> > 11) WaitLSNProcInfo / WaitLSNState
> >
> > Does this need to be exposed in xlogwait.h? These structs seem private
> > to xlogwait.c, so maybe declare it there?
> 
> Hmm, I don't remember why I moved them to xlogwait.h.  OK, moved them
> back to xlogwait.c.

This change made the code no longer compile, because
WaitLSNState->minWaitedLSN is used in xlogrecovery.c which no longer has
access to the field definition.  A rebased version with that change
reverted is attached.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Thou shalt study thy libraries and strive not to reinvent them without
cause, that thy code may be short and readable and thy days pleasant
and productive. (7th Commandment for C Programmers)

  14. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-08-07T15:00:50Z

    Hi,

Thanks for working on this.

I’ve just come across this thread and haven’t had a chance to dig into
the patch yet, but I’m keen to review it soon. In the meantime, I have
a quick question: is WAIT FOR REPLY intended mainly for user-defined
functions, or can internal code invoke it as well?

During a recent performance run [1] I noticed heavy polling in
read_local_xlog_page_guts(). Heikki’s comment from a few months ago
also hints that we could replace this check–sleep–repeat loop with the
condition-variable (CV) infrastructure used by walsender:

/*
 * Loop waiting for xlog to be available if necessary
 *
 * TODO: The walsender has its own version of this function, which uses a
 * condition variable to wake up whenever WAL is flushed. We could use the
 * same infrastructure here, instead of the check/sleep/repeat style of
 * loop.
 */

Because read_local_xlog_page_guts() waits for a specific flush or
replay LSN, polling becomes inefficient when the wait is long. I built
a POC patch that swaps polling for CVs, but a single global CV (or
even separate “flush” and “replay” CVs) isn’t ideal:

The wake-up routines don’t know which LSN each waiter cares about, so
they’d have to broadcast on every flush/replay. Caching the minimum
outstanding LSN could reduce spuriously awakened waiters, yet wouldn’t
eliminate them—multiple backends might wait for different LSNs
simultaneously. A more precise solution would require a request queue
that maps waiters to target LSNs and issues targeted wake-ups, adding
complexity.

Walsender accepts the potential broadcast overhead by using two cvs
for different waiters, so it might be acceptable for
read_local_xlog_page_guts() as well. However, if WAIT FOR REPLY
becomes available to backend code, we might leverage it to eliminate
the polling for waiting replay in read_local_xlog_page_guts() without
introducing a bespoke dispatcher. I’d appreciate any thoughts on
whether that use case is in scope.

Best,
Xuneng

[1] https://www.postgresql.org/message-id/CABPTF7VuFYm9TtA9vY8ZtS77qsT+yL_HtSDxUFnW3XsdB5b9ew@mail.gmail.com

  15. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-08-08T06:54:18Z

    Hello, Álvaro!

On Wed, Aug 6, 2025 at 6:01 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2025-Apr-29, Alexander Korotkov wrote:
>
> > > 11) WaitLSNProcInfo / WaitLSNState
> > >
> > > Does this need to be exposed in xlogwait.h? These structs seem private
> > > to xlogwait.c, so maybe declare it there?
> >
> > Hmm, I don't remember why I moved them to xlogwait.h.  OK, moved them
> > back to xlogwait.c.
>
> This change made the code no longer compile, because
> WaitLSNState->minWaitedLSN is used in xlogrecovery.c which no longer has
> access to the field definition.  A rebased version with that change
> reverted is attached.

Thank you!  The rebased version looks correct for me.

------
Regards,
Alexander Korotkov
Supabase

  16. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-08-08T07:08:49Z

    Hi, Xuneng Zhou!

On Thu, Aug 7, 2025 at 6:01 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> Thanks for working on this.
>
> I’ve just come across this thread and haven’t had a chance to dig into
> the patch yet, but I’m keen to review it soon.

Great.  Thank you for your attention to this patch.  I appreciate your
intention to review it.

> In the meantime, I have
> a quick question: is WAIT FOR REPLY intended mainly for user-defined
> functions, or can internal code invoke it as well?

Currently, WaitForLSNReplay() is assumed to only be called from
backend, as corresponding shmem is allocated only per-backend.  But
there is absolutely no problem to tweak the patch to allocate shmem
for every Postgres process.  This would enable to call
WaitForLSNReplay() wherever it is needed.  There is only no problem to
extend this approach to support other kinds of LSNs not just replay
LSN.


> During a recent performance run [1] I noticed heavy polling in
> read_local_xlog_page_guts(). Heikki’s comment from a few months ago
> also hints that we could replace this check–sleep–repeat loop with the
> condition-variable (CV) infrastructure used by walsender:
>
> /*
>  * Loop waiting for xlog to be available if necessary
>  *
>  * TODO: The walsender has its own version of this function, which uses a
>  * condition variable to wake up whenever WAL is flushed. We could use the
>  * same infrastructure here, instead of the check/sleep/repeat style of
>  * loop.
>  */
>
> Because read_local_xlog_page_guts() waits for a specific flush or
> replay LSN, polling becomes inefficient when the wait is long. I built
> a POC patch that swaps polling for CVs, but a single global CV (or
> even separate “flush” and “replay” CVs) isn’t ideal:
>
> The wake-up routines don’t know which LSN each waiter cares about, so
> they’d have to broadcast on every flush/replay. Caching the minimum
> outstanding LSN could reduce spuriously awakened waiters, yet wouldn’t
> eliminate them—multiple backends might wait for different LSNs
> simultaneously. A more precise solution would require a request queue
> that maps waiters to target LSNs and issues targeted wake-ups, adding
> complexity.
>
> Walsender accepts the potential broadcast overhead by using two cvs
> for different waiters, so it might be acceptable for
> read_local_xlog_page_guts() as well. However, if WAIT FOR REPLY
> becomes available to backend code, we might leverage it to eliminate
> the polling for waiting replay in read_local_xlog_page_guts() without
> introducing a bespoke dispatcher. I’d appreciate any thoughts on
> whether that use case is in scope.

This looks like a great new use-case for facilities developed in this
patch!  I'll remove the restriction to use WaitForLSNReplay() only in
backend.  I think you can write a patch with additional pairing heap
for flush LSN and include that into thread about
read_local_xlog_page_guts() optimization.  Let me know if you need any
assistance.

------
Regards,
Alexander Korotkov
Supabase

  17. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-08-09T10:52:37Z

    Hi Alexander!

> > In the meantime, I have
> > a quick question: is WAIT FOR REPLY intended mainly for user-defined
> > functions, or can internal code invoke it as well?
>
> Currently, WaitForLSNReplay() is assumed to only be called from
> backend, as corresponding shmem is allocated only per-backend.  But
> there is absolutely no problem to tweak the patch to allocate shmem
> for every Postgres process.  This would enable to call
> WaitForLSNReplay() wherever it is needed.  There is only no problem to
> extend this approach to support other kinds of LSNs not just replay
> LSN.

Thanks for extending the functionality of the Wait For Replay patch!

> This looks like a great new use-case for facilities developed in this
> patch!  I'll remove the restriction to use WaitForLSNReplay() only in
> backend.  I think you can write a patch with additional pairing heap
> for flush LSN and include that into thread about
> read_local_xlog_page_guts() optimization.  Let me know if you need any
> assistance.

This could be a more elegant approach which would solve the polling
issue well. I'll prepare a follow-up patch for it.

Best,
Xuneng

  18. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-08-09T11:27:25Z

    Hi,

> On Thu, Aug 7, 2025 at 6:01 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > Thanks for working on this.
> >
> > I’ve just come across this thread and haven’t had a chance to dig into
> > the patch yet, but I’m keen to review it soon.
>
> Great.  Thank you for your attention to this patch.  I appreciate your
> intention to review it.

I did a quick pass over v7. There are a few thoughts to share—mostly
around documentation, build, and tests, plus some minor nits. The core
logic looks solid to me. I’ll take a deeper look as I work on a
follow‑up patch to add waiting for flush LSNs. And the patch seems to
need rebase; it can't be applied to HEAD cleanly for now.

Build
1) Consider adding a comma in `src/test/recovery/meson.build` after
`'t/048_vacuum_horizon_floor.pl'` so the list remains valid.

Core code
2) It may be safer for `WaitLSNWakeup()` to assert against the stack array size:
) Perhaps `Assert(numWakeUpProcs < WAKEUP_PROC_STATIC_ARRAY_SIZE);`
rather than `MaxBackends`.
For option parsing UX in `wait.c`, we might prefer:
3) Using `ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR),
errmsg(...)))` instead of `elog(ERROR, ...)` for consistency and
translatability.
4) Explicitly rejecting duplicate `LSN`/`TIMEOUT` options with a syntax error.
5) The result column label could align better with other utility
outputs if shortened to `status` (lowercase, no space).
6) After `parse_real()`, it could help to validate/clamp the timeout
to avoid overflow when converting to `int64` and when passing a `long`
to `WaitLatch()`.
7) If `nodes/print.h` in `src/backend/commands/wait.c` isn’t used, we
might drop the include.
8) A couple of comment nits: “do it this outside” → “do this outside”.

Tests
9) We might consider adding cases for:
- Negative `TIMEOUT` (to exercise the error path).
- Syntax errors (unknown option; duplicate `LSN`/`TIMEOUT`; missing `LSN`).

Documentation
`doc/src/sgml/ref/wait_for.sgml`
10) The index term could be updated to `<primary>WAIT FOR</primary>`.
11) The synopsis might read more clearly as:
- WAIT FOR LSN '<lsn>' [ TIMEOUT <milliseconds |
'duration-with-units'> ] [ NO_THROW ]
12) The purpose line might be smoother as “wait for a target LSN to be
replayed, optionally with a timeout”.
13) Return values might use `<literal>` for `success`, `timeout`, `not
in recovery`.
14) Consistently calling this a “command” (rather than
function/procedure) could reduce confusion.
15) The example text might read more cleanly as “If the target LSN is
not reached before the timeout …”.

`doc/src/sgml/high-availability.sgml`
16) The sentence could read “However, it is possible to address this
without switching to synchronous replication.”

`src/backend/utils/activity/wait_event_names.txt`
17) The description for `WAIT_FOR_WAL_REPLAY` might be clearer as
“Waiting for WAL replay to reach a target LSN on a standby.”

Best,
Xuneng

  19. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-08-27T15:54:25Z

    Hi all,

I did a rebase for the patch to v8 and incorporated a few changes:

1) Updated documentation, added new tests, and applied minor code
adjustments based on prior review comments.
2) Tweaked the initialization of waitReplayLSNState so that
non-backend processes can call wait for replay.

Started a new thread [1] and attached a patch addressing the polling
issue in the function
read_local_xlog_page_guts built on the infra of patch v8.

[1] https://www.postgresql.org/message-id/CABPTF7Vr99gZ5GM_ZYbYnd9MMnoVW3pukBEviVoHKRvJW-dE3g@mail.gmail.com

Feedbacks welcome.

Best,
Xuneng

  20. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-09-13T19:31:32Z

    Hi, Xuneng!

On Wed, Aug 27, 2025 at 6:54 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> I did a rebase for the patch to v8 and incorporated a few changes:
>
> 1) Updated documentation, added new tests, and applied minor code
> adjustments based on prior review comments.
> 2) Tweaked the initialization of waitReplayLSNState so that
> non-backend processes can call wait for replay.
>
> Started a new thread [1] and attached a patch addressing the polling
> issue in the function
> read_local_xlog_page_guts built on the infra of patch v8.
>
> [1] https://www.postgresql.org/message-id/CABPTF7Vr99gZ5GM_ZYbYnd9MMnoVW3pukBEviVoHKRvJW-dE3g@mail.gmail.com
>
> Feedbacks welcome.

Thank you for your reviewing and revising this patch.

I see you've integrated most of your points expressed in [1].  I went
though them and I've integrated the rest of them.  Except this one.

> 11) The synopsis might read more clearly as:
> - WAIT FOR LSN '<lsn>' [ TIMEOUT <milliseconds | 'duration-with-units'> ] [ NO_THROW ]

I didn't find examples on how we do the similar things on other places
of docs.  This is why I decided to leave this place as it currently
is.

Also, I found some mess up with typedefs.list.  I've returned the
changes to typdefs.list back and re-indented the sources.

I'd like to ask your opinion of the way this feature is implemented in
terms of grammar: generic parsing implemented in gram.y and the rest
is done in wait.c.  I think this approach should minimize additional
keywords and states for parsing code.  This comes at the price of more
complex code in wait.c, but I think this is a fair price.

Links.
1. https://www.postgresql.org/message-id/CABPTF7VsoGDMBq34MpLrMSZyxNZvVbgH6-zxtJOg5AwOoYURbw%40mail.gmail.com

------
Regards,
Alexander Korotkov
Supabase

  21. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-09-14T13:51:21Z

    Hi Alexander,

On Sun, Sep 14, 2025 at 3:31 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> Hi, Xuneng!
>
> On Wed, Aug 27, 2025 at 6:54 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > I did a rebase for the patch to v8 and incorporated a few changes:
> >
> > 1) Updated documentation, added new tests, and applied minor code
> > adjustments based on prior review comments.
> > 2) Tweaked the initialization of waitReplayLSNState so that
> > non-backend processes can call wait for replay.
> >
> > Started a new thread [1] and attached a patch addressing the polling
> > issue in the function
> > read_local_xlog_page_guts built on the infra of patch v8.
> >
> > [1] https://www.postgresql.org/message-id/CABPTF7Vr99gZ5GM_ZYbYnd9MMnoVW3pukBEviVoHKRvJW-dE3g@mail.gmail.com
> >
> > Feedbacks welcome.
>
> Thank you for your reviewing and revising this patch.
>
> I see you've integrated most of your points expressed in [1].  I went
> though them and I've integrated the rest of them.  Except this one.
>
> > 11) The synopsis might read more clearly as:
> > - WAIT FOR LSN '<lsn>' [ TIMEOUT <milliseconds | 'duration-with-units'> ] [ NO_THROW ]
>
> I didn't find examples on how we do the similar things on other places
> of docs.  This is why I decided to leave this place as it currently
> is.

+1. I re-check other commands with similar parameter patterns, and
they follow the approach in v9.

>
> Also, I found some mess up with typedefs.list.  I've returned the
> changes to typdefs.list back and re-indented the sources.

 Thanks for catching and fixing that.

> I'd like to ask your opinion of the way this feature is implemented in
> terms of grammar: generic parsing implemented in gram.y and the rest
> is done in wait.c.  I think this approach should minimize additional
> keywords and states for parsing code.  This comes at the price of more
> complex code in wait.c, but I think this is a fair price.

It's LGTM. The same pattern is observed in VACUUM, EXPLAIN, and CREATE
PUBLICATION - all use minimal grammar rules that produce generic
option lists, with the actual interpretation done in their respective
implementation files. The moderate complexity in wait.c seems
acceptable.

Best,
Xuneng

  22. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-09-15T18:59:42Z

    Hi, Xuneng!

On Sun, Sep 14, 2025 at 4:51 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> On Sun, Sep 14, 2025 at 3:31 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > On Wed, Aug 27, 2025 at 6:54 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > I did a rebase for the patch to v8 and incorporated a few changes:
> > >
> > > 1) Updated documentation, added new tests, and applied minor code
> > > adjustments based on prior review comments.
> > > 2) Tweaked the initialization of waitReplayLSNState so that
> > > non-backend processes can call wait for replay.
> > >
> > > Started a new thread [1] and attached a patch addressing the polling
> > > issue in the function
> > > read_local_xlog_page_guts built on the infra of patch v8.
> > >
> > > [1] https://www.postgresql.org/message-id/CABPTF7Vr99gZ5GM_ZYbYnd9MMnoVW3pukBEviVoHKRvJW-dE3g@mail.gmail.com
> > >
> > > Feedbacks welcome.
> >
> > Thank you for your reviewing and revising this patch.
> >
> > I see you've integrated most of your points expressed in [1].  I went
> > though them and I've integrated the rest of them.  Except this one.
> >
> > > 11) The synopsis might read more clearly as:
> > > - WAIT FOR LSN '<lsn>' [ TIMEOUT <milliseconds | 'duration-with-units'> ] [ NO_THROW ]
> >
> > I didn't find examples on how we do the similar things on other places
> > of docs.  This is why I decided to leave this place as it currently
> > is.
>
> +1. I re-check other commands with similar parameter patterns, and
> they follow the approach in v9.
>
> >
> > Also, I found some mess up with typedefs.list.  I've returned the
> > changes to typdefs.list back and re-indented the sources.
>
>  Thanks for catching and fixing that.
>
> > I'd like to ask your opinion of the way this feature is implemented in
> > terms of grammar: generic parsing implemented in gram.y and the rest
> > is done in wait.c.  I think this approach should minimize additional
> > keywords and states for parsing code.  This comes at the price of more
> > complex code in wait.c, but I think this is a fair price.
>
> It's LGTM. The same pattern is observed in VACUUM, EXPLAIN, and CREATE
> PUBLICATION - all use minimal grammar rules that produce generic
> option lists, with the actual interpretation done in their respective
> implementation files. The moderate complexity in wait.c seems
> acceptable.

The attached revision of patch contains fix of the typo in the comment
you reported off-list.

------
Regards,
Alexander Korotkov
Supabase

  23. Re: Implement waiting for wal lsn replay: reloaded

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-15T20:24:05Z

    On 2025-Sep-15, Alexander Korotkov wrote:

> > It's LGTM. The same pattern is observed in VACUUM, EXPLAIN, and CREATE
> > PUBLICATION - all use minimal grammar rules that produce generic
> > option lists, with the actual interpretation done in their respective
> > implementation files. The moderate complexity in wait.c seems
> > acceptable.

Actually I find the code in ExecWaitStmt pretty unusual.  We tend to use
lists of DefElem (a name optionally followed by a value) instead of
individual scattered elements that must later be matched up.  Why not
use utility_option_list instead and then loop on the list of DefElems?
It'd be a lot simpler.

Also, we've found that failing to surround the options by parens leads
to pain down the road, so maybe add that.  Given that the LSN seems to
be mandatory, maybe make it something like

WAIT FOR LSN 'xy/zzy' [ WITH ( utility_option_list ) ]

This requires that you make LSN a keyword, albeit unreserved.  Or you
could make it
WAIT FOR Ident [the rest]
and then ensure in C that the identifier matches the word LSN, such as
we do for "permissive" and "restrictive" in
RowSecurityDefaultPermissive.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

  24. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-09-26T11:22:42Z

    Hi Álvaro,

Thanks for your review.

On Tue, Sep 16, 2025 at 4:24 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2025-Sep-15, Alexander Korotkov wrote:
>
> > > It's LGTM. The same pattern is observed in VACUUM, EXPLAIN, and CREATE
> > > PUBLICATION - all use minimal grammar rules that produce generic
> > > option lists, with the actual interpretation done in their respective
> > > implementation files. The moderate complexity in wait.c seems
> > > acceptable.
>
> Actually I find the code in ExecWaitStmt pretty unusual.  We tend to use
> lists of DefElem (a name optionally followed by a value) instead of
> individual scattered elements that must later be matched up.  Why not
> use utility_option_list instead and then loop on the list of DefElems?
> It'd be a lot simpler.

I took a look at commands like VACUUM and EXPLAIN and they do follow
this pattern. v11 will make use of utility_option_list.

> Also, we've found that failing to surround the options by parens leads
> to pain down the road, so maybe add that.  Given that the LSN seems to
> be mandatory, maybe make it something like
>
> WAIT FOR LSN 'xy/zzy' [ WITH ( utility_option_list ) ]
>
> This requires that you make LSN a keyword, albeit unreserved.  Or you
> could make it
> WAIT FOR Ident [the rest]
> and then ensure in C that the identifier matches the word LSN, such as
> we do for "permissive" and "restrictive" in
> RowSecurityDefaultPermissive.

Shall make LSN an unreserved keyword as well.

Best,
Xuneng

  25. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-09-28T09:02:43Z

    Hi,

On Fri, Sep 26, 2025 at 7:22 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi Álvaro,
>
> Thanks for your review.
>
> On Tue, Sep 16, 2025 at 4:24 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> >
> > On 2025-Sep-15, Alexander Korotkov wrote:
> >
> > > > It's LGTM. The same pattern is observed in VACUUM, EXPLAIN, and CREATE
> > > > PUBLICATION - all use minimal grammar rules that produce generic
> > > > option lists, with the actual interpretation done in their respective
> > > > implementation files. The moderate complexity in wait.c seems
> > > > acceptable.
> >
> > Actually I find the code in ExecWaitStmt pretty unusual.  We tend to use
> > lists of DefElem (a name optionally followed by a value) instead of
> > individual scattered elements that must later be matched up.  Why not
> > use utility_option_list instead and then loop on the list of DefElems?
> > It'd be a lot simpler.
>
> I took a look at commands like VACUUM and EXPLAIN and they do follow
> this pattern. v11 will make use of utility_option_list.
>
> > Also, we've found that failing to surround the options by parens leads
> > to pain down the road, so maybe add that.  Given that the LSN seems to
> > be mandatory, maybe make it something like
> >
> > WAIT FOR LSN 'xy/zzy' [ WITH ( utility_option_list ) ]
> >
> > This requires that you make LSN a keyword, albeit unreserved.  Or you
> > could make it
> > WAIT FOR Ident [the rest]
> > and then ensure in C that the identifier matches the word LSN, such as
> > we do for "permissive" and "restrictive" in
> > RowSecurityDefaultPermissive.
>
> Shall make LSN an unreserved keyword as well.

Here's the updated v11.  Many thanks Jian for off-list discussions and review.

Best,
Xuneng

  26. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-10-04T01:35:32Z

    Hi,

On Sun, Sep 28, 2025 at 5:02 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> On Fri, Sep 26, 2025 at 7:22 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi Álvaro,
> >
> > Thanks for your review.
> >
> > On Tue, Sep 16, 2025 at 4:24 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > >
> > > On 2025-Sep-15, Alexander Korotkov wrote:
> > >
> > > > > It's LGTM. The same pattern is observed in VACUUM, EXPLAIN, and CREATE
> > > > > PUBLICATION - all use minimal grammar rules that produce generic
> > > > > option lists, with the actual interpretation done in their respective
> > > > > implementation files. The moderate complexity in wait.c seems
> > > > > acceptable.
> > >
> > > Actually I find the code in ExecWaitStmt pretty unusual.  We tend to use
> > > lists of DefElem (a name optionally followed by a value) instead of
> > > individual scattered elements that must later be matched up.  Why not
> > > use utility_option_list instead and then loop on the list of DefElems?
> > > It'd be a lot simpler.
> >
> > I took a look at commands like VACUUM and EXPLAIN and they do follow
> > this pattern. v11 will make use of utility_option_list.
> >
> > > Also, we've found that failing to surround the options by parens leads
> > > to pain down the road, so maybe add that.  Given that the LSN seems to
> > > be mandatory, maybe make it something like
> > >
> > > WAIT FOR LSN 'xy/zzy' [ WITH ( utility_option_list ) ]
> > >
> > > This requires that you make LSN a keyword, albeit unreserved.  Or you
> > > could make it
> > > WAIT FOR Ident [the rest]
> > > and then ensure in C that the identifier matches the word LSN, such as
> > > we do for "permissive" and "restrictive" in
> > > RowSecurityDefaultPermissive.
> >
> > Shall make LSN an unreserved keyword as well.
>
> Here's the updated v11.  Many thanks Jian for off-list discussions and review.

v12 removed unused
+WaitStmt
+WaitStmtParam in pgindent/typedefs.list.

Best,
Xuneng

  27. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-10-14T13:03:30Z

    Hi,

On Sat, Oct 4, 2025 at 9:35 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> On Sun, Sep 28, 2025 at 5:02 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi,
> >
> > On Fri, Sep 26, 2025 at 7:22 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > >
> > > Hi Álvaro,
> > >
> > > Thanks for your review.
> > >
> > > On Tue, Sep 16, 2025 at 4:24 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > > >
> > > > On 2025-Sep-15, Alexander Korotkov wrote:
> > > >
> > > > > > It's LGTM. The same pattern is observed in VACUUM, EXPLAIN, and CREATE
> > > > > > PUBLICATION - all use minimal grammar rules that produce generic
> > > > > > option lists, with the actual interpretation done in their respective
> > > > > > implementation files. The moderate complexity in wait.c seems
> > > > > > acceptable.
> > > >
> > > > Actually I find the code in ExecWaitStmt pretty unusual.  We tend to use
> > > > lists of DefElem (a name optionally followed by a value) instead of
> > > > individual scattered elements that must later be matched up.  Why not
> > > > use utility_option_list instead and then loop on the list of DefElems?
> > > > It'd be a lot simpler.
> > >
> > > I took a look at commands like VACUUM and EXPLAIN and they do follow
> > > this pattern. v11 will make use of utility_option_list.
> > >
> > > > Also, we've found that failing to surround the options by parens leads
> > > > to pain down the road, so maybe add that.  Given that the LSN seems to
> > > > be mandatory, maybe make it something like
> > > >
> > > > WAIT FOR LSN 'xy/zzy' [ WITH ( utility_option_list ) ]
> > > >
> > > > This requires that you make LSN a keyword, albeit unreserved.  Or you
> > > > could make it
> > > > WAIT FOR Ident [the rest]
> > > > and then ensure in C that the identifier matches the word LSN, such as
> > > > we do for "permissive" and "restrictive" in
> > > > RowSecurityDefaultPermissive.
> > >
> > > Shall make LSN an unreserved keyword as well.
> >
> > Here's the updated v11.  Many thanks Jian for off-list discussions and review.
>
> v12 removed unused
> +WaitStmt
> +WaitStmtParam in pgindent/typedefs.list.
>

Hi, I’ve split the patch into multiple patch sets for easier review,
per Michael’s advice [1].

[1] https://www.postgresql.org/message-id/aOMsv9TszlB1n-W7%40paquier.xyz

Best,
Xuneng

  28. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-10-15T00:23:09Z

    Hi,

On Tue, Oct 14, 2025 at 9:03 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> On Sat, Oct 4, 2025 at 9:35 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi,
> >
> > On Sun, Sep 28, 2025 at 5:02 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > On Fri, Sep 26, 2025 at 7:22 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > >
> > > > Hi Álvaro,
> > > >
> > > > Thanks for your review.
> > > >
> > > > On Tue, Sep 16, 2025 at 4:24 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > > > >
> > > > > On 2025-Sep-15, Alexander Korotkov wrote:
> > > > >
> > > > > > > It's LGTM. The same pattern is observed in VACUUM, EXPLAIN, and CREATE
> > > > > > > PUBLICATION - all use minimal grammar rules that produce generic
> > > > > > > option lists, with the actual interpretation done in their respective
> > > > > > > implementation files. The moderate complexity in wait.c seems
> > > > > > > acceptable.
> > > > >
> > > > > Actually I find the code in ExecWaitStmt pretty unusual.  We tend to use
> > > > > lists of DefElem (a name optionally followed by a value) instead of
> > > > > individual scattered elements that must later be matched up.  Why not
> > > > > use utility_option_list instead and then loop on the list of DefElems?
> > > > > It'd be a lot simpler.
> > > >
> > > > I took a look at commands like VACUUM and EXPLAIN and they do follow
> > > > this pattern. v11 will make use of utility_option_list.
> > > >
> > > > > Also, we've found that failing to surround the options by parens leads
> > > > > to pain down the road, so maybe add that.  Given that the LSN seems to
> > > > > be mandatory, maybe make it something like
> > > > >
> > > > > WAIT FOR LSN 'xy/zzy' [ WITH ( utility_option_list ) ]
> > > > >
> > > > > This requires that you make LSN a keyword, albeit unreserved.  Or you
> > > > > could make it
> > > > > WAIT FOR Ident [the rest]
> > > > > and then ensure in C that the identifier matches the word LSN, such as
> > > > > we do for "permissive" and "restrictive" in
> > > > > RowSecurityDefaultPermissive.
> > > >
> > > > Shall make LSN an unreserved keyword as well.
> > >
> > > Here's the updated v11.  Many thanks Jian for off-list discussions and review.
> >
> > v12 removed unused
> > +WaitStmt
> > +WaitStmtParam in pgindent/typedefs.list.
> >
>
> Hi, I’ve split the patch into multiple patch sets for easier review,
> per Michael’s advice [1].
>
> [1] https://www.postgresql.org/message-id/aOMsv9TszlB1n-W7%40paquier.xyz
>

Patch 2 in v13 is corrupted and patch 3 has an error. Sorry for the
noise. Here's v14.

Best,
Xuneng

  29. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-10-15T08:40:03Z

    Hi,

On Wed, Oct 15, 2025 at 8:23 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> On Tue, Oct 14, 2025 at 9:03 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi,
> >
> > On Sat, Oct 4, 2025 at 9:35 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > On Sun, Sep 28, 2025 at 5:02 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Fri, Sep 26, 2025 at 7:22 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > > >
> > > > > Hi Álvaro,
> > > > >
> > > > > Thanks for your review.
> > > > >
> > > > > On Tue, Sep 16, 2025 at 4:24 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > > > > >
> > > > > > On 2025-Sep-15, Alexander Korotkov wrote:
> > > > > >
> > > > > > > > It's LGTM. The same pattern is observed in VACUUM, EXPLAIN, and CREATE
> > > > > > > > PUBLICATION - all use minimal grammar rules that produce generic
> > > > > > > > option lists, with the actual interpretation done in their respective
> > > > > > > > implementation files. The moderate complexity in wait.c seems
> > > > > > > > acceptable.
> > > > > >
> > > > > > Actually I find the code in ExecWaitStmt pretty unusual.  We tend to use
> > > > > > lists of DefElem (a name optionally followed by a value) instead of
> > > > > > individual scattered elements that must later be matched up.  Why not
> > > > > > use utility_option_list instead and then loop on the list of DefElems?
> > > > > > It'd be a lot simpler.
> > > > >
> > > > > I took a look at commands like VACUUM and EXPLAIN and they do follow
> > > > > this pattern. v11 will make use of utility_option_list.
> > > > >
> > > > > > Also, we've found that failing to surround the options by parens leads
> > > > > > to pain down the road, so maybe add that.  Given that the LSN seems to
> > > > > > be mandatory, maybe make it something like
> > > > > >
> > > > > > WAIT FOR LSN 'xy/zzy' [ WITH ( utility_option_list ) ]
> > > > > >
> > > > > > This requires that you make LSN a keyword, albeit unreserved.  Or you
> > > > > > could make it
> > > > > > WAIT FOR Ident [the rest]
> > > > > > and then ensure in C that the identifier matches the word LSN, such as
> > > > > > we do for "permissive" and "restrictive" in
> > > > > > RowSecurityDefaultPermissive.
> > > > >
> > > > > Shall make LSN an unreserved keyword as well.
> > > >
> > > > Here's the updated v11.  Many thanks Jian for off-list discussions and review.
> > >
> > > v12 removed unused
> > > +WaitStmt
> > > +WaitStmtParam in pgindent/typedefs.list.
> > >
> >
> > Hi, I’ve split the patch into multiple patch sets for easier review,
> > per Michael’s advice [1].
> >
> > [1] https://www.postgresql.org/message-id/aOMsv9TszlB1n-W7%40paquier.xyz
> >
>
> Patch 2 in v13 is corrupted and patch 3 has an error. Sorry for the
> noise. Here's v14.
>

Made minor changes to #include of xlogwait.h in patch2 to calm CF-bots down.

Best,
Xuneng

  30. Re: Implement waiting for wal lsn replay: reloaded

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-10-15T08:51:42Z

    I didn't review the patch other than look at the grammar, but I disagree
with using opt_with in it.  I think WITH should be a mandatory word, or
just not be there at all.  The current formulation lets you do one of:

1. WAIT FOR LSN '123/456' WITH (opt = val);
2. WAIT FOR LSN '123/456' (opt = val);
3. WAIT FOR LSN '123/456';

and I don't see why you need two ways to specify an option list.

So one option is to remove opt_wait_with_clause and just use
opt_utility_option_list, which would remove the WITH keyword from there
(ie. only keep 2 and 3 from the above list).  But I think that's worse:
just look at the REPACK grammar[1], where we have to have additional
productions for the optional parenthesized option list.

So why not do just

+opt_wait_with_clause:
+           WITH '(' utility_option_list ')'        { $$ = $3; }
+           | /*EMPTY*/                             { $$ = NIL; }
+           ;

which keeps options 1 and 3 of the list above.

Note: you don't need to worry about WITH_LA, because that's only going
to show up when the user writes WITH TIME or WITH ORDINALITY (see
parser.c), and that's a syntax error anyway.


[1] https://postgr.es/m/202510101352.vvp4p3p2dblu@alvherre.pgsql

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)

  31. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-10-15T12:48:29Z

    Hi,

Thank you for the grammar review and the clear recommendation.

On Wed, Oct 15, 2025 at 4:51 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> I didn't review the patch other than look at the grammar, but I disagree
> with using opt_with in it.  I think WITH should be a mandatory word, or
> just not be there at all.  The current formulation lets you do one of:
>
> 1. WAIT FOR LSN '123/456' WITH (opt = val);
> 2. WAIT FOR LSN '123/456' (opt = val);
> 3. WAIT FOR LSN '123/456';
>
> and I don't see why you need two ways to specify an option list.

I agree with this as unnecessary choices are confusing.

>
> So one option is to remove opt_wait_with_clause and just use
> opt_utility_option_list, which would remove the WITH keyword from there
> (ie. only keep 2 and 3 from the above list).  But I think that's worse:
> just look at the REPACK grammar[1], where we have to have additional
> productions for the optional parenthesized option list.
>
>
>
> So why not do just
>
> +opt_wait_with_clause:
> +           WITH '(' utility_option_list ')'        { $$ = $3; }
> +           | /*EMPTY*/                             { $$ = NIL; }
> +           ;
>
> which keeps options 1 and 3 of the list above.

Your suggested approach of making WITH mandatory when options are
present looks better.
I've implemented the change as you recommended. Please see patch 3 in v16.

>
>
>
> Note: you don't need to worry about WITH_LA, because that's only going
> to show up when the user writes WITH TIME or WITH ORDINALITY (see
> parser.c), and that's a syntax error anyway.
>

Yeah, we require '(' immediately after WITH in our grammar, the
lookahead mechanism will keep it as regular WITH, and any attempt to
write "WITH TIME" or "WITH ORDINALITY" would be a syntax error anyway,
which is expected.

Best,
Xuneng

  32. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-10-16T07:11:58Z

    Hi,

On Wed, Oct 15, 2025 at 8:48 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> Thank you for the grammar review and the clear recommendation.
>
> On Wed, Oct 15, 2025 at 4:51 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> >
> > I didn't review the patch other than look at the grammar, but I disagree
> > with using opt_with in it.  I think WITH should be a mandatory word, or
> > just not be there at all.  The current formulation lets you do one of:
> >
> > 1. WAIT FOR LSN '123/456' WITH (opt = val);
> > 2. WAIT FOR LSN '123/456' (opt = val);
> > 3. WAIT FOR LSN '123/456';
> >
> > and I don't see why you need two ways to specify an option list.
>
> I agree with this as unnecessary choices are confusing.
>
> >
> > So one option is to remove opt_wait_with_clause and just use
> > opt_utility_option_list, which would remove the WITH keyword from there
> > (ie. only keep 2 and 3 from the above list).  But I think that's worse:
> > just look at the REPACK grammar[1], where we have to have additional
> > productions for the optional parenthesized option list.
> >
> >
> >
> > So why not do just
> >
> > +opt_wait_with_clause:
> > +           WITH '(' utility_option_list ')'        { $$ = $3; }
> > +           | /*EMPTY*/                             { $$ = NIL; }
> > +           ;
> >
> > which keeps options 1 and 3 of the list above.
>
> Your suggested approach of making WITH mandatory when options are
> present looks better.
> I've implemented the change as you recommended. Please see patch 3 in v16.
>
> >
> >
> >
> > Note: you don't need to worry about WITH_LA, because that's only going
> > to show up when the user writes WITH TIME or WITH ORDINALITY (see
> > parser.c), and that's a syntax error anyway.
> >
>
> Yeah, we require '(' immediately after WITH in our grammar, the
> lookahead mechanism will keep it as regular WITH, and any attempt to
> write "WITH TIME" or "WITH ORDINALITY" would be a syntax error anyway,
> which is expected.
>

The filename of patch 1 is incorrect due to coping. Just correct it.

Best,
Xuneng

  33. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-10-23T10:46:27Z

    Hi!

In Thu, Oct 16, 2025 at 10:12 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> On Wed, Oct 15, 2025 at 8:48 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi,
> >
> > Thank you for the grammar review and the clear recommendation.
> >
> > On Wed, Oct 15, 2025 at 4:51 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > >
> > > I didn't review the patch other than look at the grammar, but I disagree
> > > with using opt_with in it.  I think WITH should be a mandatory word, or
> > > just not be there at all.  The current formulation lets you do one of:
> > >
> > > 1. WAIT FOR LSN '123/456' WITH (opt = val);
> > > 2. WAIT FOR LSN '123/456' (opt = val);
> > > 3. WAIT FOR LSN '123/456';
> > >
> > > and I don't see why you need two ways to specify an option list.
> >
> > I agree with this as unnecessary choices are confusing.
> >
> > >
> > > So one option is to remove opt_wait_with_clause and just use
> > > opt_utility_option_list, which would remove the WITH keyword from there
> > > (ie. only keep 2 and 3 from the above list).  But I think that's worse:
> > > just look at the REPACK grammar[1], where we have to have additional
> > > productions for the optional parenthesized option list.
> > >
> > >
> > >
> > > So why not do just
> > >
> > > +opt_wait_with_clause:
> > > +           WITH '(' utility_option_list ')'        { $$ = $3; }
> > > +           | /*EMPTY*/                             { $$ = NIL; }
> > > +           ;
> > >
> > > which keeps options 1 and 3 of the list above.
> >
> > Your suggested approach of making WITH mandatory when options are
> > present looks better.
> > I've implemented the change as you recommended. Please see patch 3 in v16.
> >
> > >
> > >
> > >
> > > Note: you don't need to worry about WITH_LA, because that's only going
> > > to show up when the user writes WITH TIME or WITH ORDINALITY (see
> > > parser.c), and that's a syntax error anyway.
> > >
> >
> > Yeah, we require '(' immediately after WITH in our grammar, the
> > lookahead mechanism will keep it as regular WITH, and any attempt to
> > write "WITH TIME" or "WITH ORDINALITY" would be a syntax error anyway,
> > which is expected.
> >
>
> The filename of patch 1 is incorrect due to coping. Just correct it.

Thank you for rebasing the patch.

I've revised it.  The significant changes has been made to 0002, where
I reduced the code duplication.  Also, I run pgindent and pgperltidy
and made other small improvements.
Please, check.

------
Regards,
Alexander Korotkov
Supabase

  34. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-10-23T12:58:46Z

    Hi,

On Thu, Oct 23, 2025 at 6:46 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> Hi!
>
> In Thu, Oct 16, 2025 at 10:12 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > On Wed, Oct 15, 2025 at 8:48 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > Thank you for the grammar review and the clear recommendation.
> > >
> > > On Wed, Oct 15, 2025 at 4:51 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > > >
> > > > I didn't review the patch other than look at the grammar, but I disagree
> > > > with using opt_with in it.  I think WITH should be a mandatory word, or
> > > > just not be there at all.  The current formulation lets you do one of:
> > > >
> > > > 1. WAIT FOR LSN '123/456' WITH (opt = val);
> > > > 2. WAIT FOR LSN '123/456' (opt = val);
> > > > 3. WAIT FOR LSN '123/456';
> > > >
> > > > and I don't see why you need two ways to specify an option list.
> > >
> > > I agree with this as unnecessary choices are confusing.
> > >
> > > >
> > > > So one option is to remove opt_wait_with_clause and just use
> > > > opt_utility_option_list, which would remove the WITH keyword from there
> > > > (ie. only keep 2 and 3 from the above list).  But I think that's worse:
> > > > just look at the REPACK grammar[1], where we have to have additional
> > > > productions for the optional parenthesized option list.
> > > >
> > > >
> > > >
> > > > So why not do just
> > > >
> > > > +opt_wait_with_clause:
> > > > +           WITH '(' utility_option_list ')'        { $$ = $3; }
> > > > +           | /*EMPTY*/                             { $$ = NIL; }
> > > > +           ;
> > > >
> > > > which keeps options 1 and 3 of the list above.
> > >
> > > Your suggested approach of making WITH mandatory when options are
> > > present looks better.
> > > I've implemented the change as you recommended. Please see patch 3 in v16.
> > >
> > > >
> > > >
> > > >
> > > > Note: you don't need to worry about WITH_LA, because that's only going
> > > > to show up when the user writes WITH TIME or WITH ORDINALITY (see
> > > > parser.c), and that's a syntax error anyway.
> > > >
> > >
> > > Yeah, we require '(' immediately after WITH in our grammar, the
> > > lookahead mechanism will keep it as regular WITH, and any attempt to
> > > write "WITH TIME" or "WITH ORDINALITY" would be a syntax error anyway,
> > > which is expected.
> > >
> >
> > The filename of patch 1 is incorrect due to coping. Just correct it.
>
> Thank you for rebasing the patch.
>
> I've revised it.  The significant changes has been made to 0002, where
> I reduced the code duplication.  Also, I run pgindent and pgperltidy
> and made other small improvements.
> Please, check.

Thanks for updating the patch set!
Patch 2 looks more elegant after the revision. I’ll review them soon.

Best,
Xuneng

  35. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-11-02T06:24:48Z

    Hi, Alexander!

On Thu, Oct 23, 2025 at 8:58 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> On Thu, Oct 23, 2025 at 6:46 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >
> > Hi!
> >
> > In Thu, Oct 16, 2025 at 10:12 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > On Wed, Oct 15, 2025 at 8:48 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > Thank you for the grammar review and the clear recommendation.
> > > >
> > > > On Wed, Oct 15, 2025 at 4:51 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > > > >
> > > > > I didn't review the patch other than look at the grammar, but I disagree
> > > > > with using opt_with in it.  I think WITH should be a mandatory word, or
> > > > > just not be there at all.  The current formulation lets you do one of:
> > > > >
> > > > > 1. WAIT FOR LSN '123/456' WITH (opt = val);
> > > > > 2. WAIT FOR LSN '123/456' (opt = val);
> > > > > 3. WAIT FOR LSN '123/456';
> > > > >
> > > > > and I don't see why you need two ways to specify an option list.
> > > >
> > > > I agree with this as unnecessary choices are confusing.
> > > >
> > > > >
> > > > > So one option is to remove opt_wait_with_clause and just use
> > > > > opt_utility_option_list, which would remove the WITH keyword from there
> > > > > (ie. only keep 2 and 3 from the above list).  But I think that's worse:
> > > > > just look at the REPACK grammar[1], where we have to have additional
> > > > > productions for the optional parenthesized option list.
> > > > >
> > > > >
> > > > >
> > > > > So why not do just
> > > > >
> > > > > +opt_wait_with_clause:
> > > > > +           WITH '(' utility_option_list ')'        { $$ = $3; }
> > > > > +           | /*EMPTY*/                             { $$ = NIL; }
> > > > > +           ;
> > > > >
> > > > > which keeps options 1 and 3 of the list above.
> > > >
> > > > Your suggested approach of making WITH mandatory when options are
> > > > present looks better.
> > > > I've implemented the change as you recommended. Please see patch 3 in v16.
> > > >
> > > > >
> > > > >
> > > > >
> > > > > Note: you don't need to worry about WITH_LA, because that's only going
> > > > > to show up when the user writes WITH TIME or WITH ORDINALITY (see
> > > > > parser.c), and that's a syntax error anyway.
> > > > >
> > > >
> > > > Yeah, we require '(' immediately after WITH in our grammar, the
> > > > lookahead mechanism will keep it as regular WITH, and any attempt to
> > > > write "WITH TIME" or "WITH ORDINALITY" would be a syntax error anyway,
> > > > which is expected.
> > > >
> > >
> > > The filename of patch 1 is incorrect due to coping. Just correct it.
> >
> > Thank you for rebasing the patch.
> >
> > I've revised it.  The significant changes has been made to 0002, where
> > I reduced the code duplication.  Also, I run pgindent and pgperltidy
> > and made other small improvements.
> > Please, check.
>
> Thanks for updating the patch set!
> Patch 2 looks more elegant after the revision. I’ll review them soon.

I’ve made a few minor updates to the comments and docs in patches 2
and 3. The patch set LGTM now.

Best,
Xuneng

  36. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-11-03T02:20:28Z

    Hi,

On Sun, Nov 2, 2025 at 2:24 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi, Alexander!
>
> On Thu, Oct 23, 2025 at 8:58 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi,
> >
> > On Thu, Oct 23, 2025 at 6:46 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > >
> > > Hi!
> > >
> > > In Thu, Oct 16, 2025 at 10:12 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > > On Wed, Oct 15, 2025 at 8:48 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > Thank you for the grammar review and the clear recommendation.
> > > > >
> > > > > On Wed, Oct 15, 2025 at 4:51 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > > > > >
> > > > > > I didn't review the patch other than look at the grammar, but I disagree
> > > > > > with using opt_with in it.  I think WITH should be a mandatory word, or
> > > > > > just not be there at all.  The current formulation lets you do one of:
> > > > > >
> > > > > > 1. WAIT FOR LSN '123/456' WITH (opt = val);
> > > > > > 2. WAIT FOR LSN '123/456' (opt = val);
> > > > > > 3. WAIT FOR LSN '123/456';
> > > > > >
> > > > > > and I don't see why you need two ways to specify an option list.
> > > > >
> > > > > I agree with this as unnecessary choices are confusing.
> > > > >
> > > > > >
> > > > > > So one option is to remove opt_wait_with_clause and just use
> > > > > > opt_utility_option_list, which would remove the WITH keyword from there
> > > > > > (ie. only keep 2 and 3 from the above list).  But I think that's worse:
> > > > > > just look at the REPACK grammar[1], where we have to have additional
> > > > > > productions for the optional parenthesized option list.
> > > > > >
> > > > > >
> > > > > >
> > > > > > So why not do just
> > > > > >
> > > > > > +opt_wait_with_clause:
> > > > > > +           WITH '(' utility_option_list ')'        { $$ = $3; }
> > > > > > +           | /*EMPTY*/                             { $$ = NIL; }
> > > > > > +           ;
> > > > > >
> > > > > > which keeps options 1 and 3 of the list above.
> > > > >
> > > > > Your suggested approach of making WITH mandatory when options are
> > > > > present looks better.
> > > > > I've implemented the change as you recommended. Please see patch 3 in v16.
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > Note: you don't need to worry about WITH_LA, because that's only going
> > > > > > to show up when the user writes WITH TIME or WITH ORDINALITY (see
> > > > > > parser.c), and that's a syntax error anyway.
> > > > > >
> > > > >
> > > > > Yeah, we require '(' immediately after WITH in our grammar, the
> > > > > lookahead mechanism will keep it as regular WITH, and any attempt to
> > > > > write "WITH TIME" or "WITH ORDINALITY" would be a syntax error anyway,
> > > > > which is expected.
> > > > >
> > > >
> > > > The filename of patch 1 is incorrect due to coping. Just correct it.
> > >
> > > Thank you for rebasing the patch.
> > >
> > > I've revised it.  The significant changes has been made to 0002, where
> > > I reduced the code duplication.  Also, I run pgindent and pgperltidy
> > > and made other small improvements.
> > > Please, check.
> >
> > Thanks for updating the patch set!
> > Patch 2 looks more elegant after the revision. I’ll review them soon.
>
> I’ve made a few minor updates to the comments and docs in patches 2
> and 3. The patch set LGTM now.

Fix an minor issue in v18: WaitStmt was mistakenly added to
pgindent/typedefs.list in patch 2, but it should belong to patch 3.

Best,
Xuneng

  37. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-11-03T11:46:40Z

    Hello, Xuneng!

On Mon, Nov 3, 2025 at 4:20 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> On Sun, Nov 2, 2025 at 2:24 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > On Thu, Oct 23, 2025 at 8:58 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > I’ve made a few minor updates to the comments and docs in patches 2
> > and 3. The patch set LGTM now.
>
> Fix an minor issue in v18: WaitStmt was mistakenly added to
> pgindent/typedefs.list in patch 2, but it should belong to patch 3.

Thank you.  I also made some minor changes to 0002 renaming
"operation" => "lsnType".

I'd like to give this subject another chance for pg19.  I'm going to
push this if no objections.

------
Regards,
Alexander Korotkov
Supabase

  38. Re: Implement waiting for wal lsn replay: reloaded

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-03T15:06:58Z

    On 2025-Nov-03, Alexander Korotkov wrote:

> I'd like to give this subject another chance for pg19.  I'm going to
> push this if no objections.

Sure.  I don't understand why patches 0002 and 0003 are separate though.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

  39. Re: Implement waiting for wal lsn replay: reloaded

    Andres Freund <andres@anarazel.de> — 2025-11-03T15:13:02Z

    Hi,

On 2025-11-03 16:06:58 +0100, Álvaro Herrera wrote:
> On 2025-Nov-03, Alexander Korotkov wrote:
> 
> > I'd like to give this subject another chance for pg19.  I'm going to
> > push this if no objections.
> 
> Sure.  I don't understand why patches 0002 and 0003 are separate though.

FWIW, I appreciate such splits. Even if the functionality isn't usable
independently, it's still different type of code that's affected. And the
patches are each big enough to make that worthwhile for easier review.

One thing that'd be nice to do once we have WAIT FOR is to make the common
case of wait_for_catchup() use this facility, instead of polling...

Greetings,

Andres Freund

  40. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-11-05T09:51:18Z

    Hi!

On Mon, Nov 3, 2025 at 5:13 PM Andres Freund <andres@anarazel.de> wrote:
> On 2025-11-03 16:06:58 +0100, Álvaro Herrera wrote:
> > On 2025-Nov-03, Alexander Korotkov wrote:
> >
> > > I'd like to give this subject another chance for pg19.  I'm going to
> > > push this if no objections.
> >
> > Sure.  I don't understand why patches 0002 and 0003 are separate though.
>
> FWIW, I appreciate such splits. Even if the functionality isn't usable
> independently, it's still different type of code that's affected. And the
> patches are each big enough to make that worthwhile for easier review.

Thank you for the feedback, pushed.

> One thing that'd be nice to do once we have WAIT FOR is to make the common
> case of wait_for_catchup() use this facility, instead of polling...

The draft patch for that is attached.  WAIT FOR doesn't handle all the
possible use cases of wait_for_catchup(), but I've added usage when
it's appropriate.

------
Regards,
Alexander Korotkov
Supabase

  41. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-11-05T14:03:25Z

    Hi,

On Wed, Nov 5, 2025 at 5:51 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> Hi!
>
> On Mon, Nov 3, 2025 at 5:13 PM Andres Freund <andres@anarazel.de> wrote:
> > On 2025-11-03 16:06:58 +0100, Álvaro Herrera wrote:
> > > On 2025-Nov-03, Alexander Korotkov wrote:
> > >
> > > > I'd like to give this subject another chance for pg19.  I'm going to
> > > > push this if no objections.
> > >
> > > Sure.  I don't understand why patches 0002 and 0003 are separate though.
> >
> > FWIW, I appreciate such splits. Even if the functionality isn't usable
> > independently, it's still different type of code that's affected. And the
> > patches are each big enough to make that worthwhile for easier review.
>
> Thank you for the feedback, pushed.

Thanks for pushing them!

> > One thing that'd be nice to do once we have WAIT FOR is to make the common
> > case of wait_for_catchup() use this facility, instead of polling...
>
> The draft patch for that is attached.  WAIT FOR doesn't handle all the
> possible use cases of wait_for_catchup(), but I've added usage when
> it's appropriate.

Interesting, could this approach be extended to the flush and other
modes as well? I might need to spend some time to understand it before
I can provide a meaningful review.

Best,
Xuneng

  42. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-11-07T22:02:36Z

    On Wed, Nov 5, 2025 at 4:03 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> On Wed, Nov 5, 2025 at 5:51 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > On Mon, Nov 3, 2025 at 5:13 PM Andres Freund <andres@anarazel.de> wrote:
> > > On 2025-11-03 16:06:58 +0100, Álvaro Herrera wrote:
> > > > On 2025-Nov-03, Alexander Korotkov wrote:
> > > >
> > > > > I'd like to give this subject another chance for pg19.  I'm going to
> > > > > push this if no objections.
> > > >
> > > > Sure.  I don't understand why patches 0002 and 0003 are separate though.
> > >
> > > FWIW, I appreciate such splits. Even if the functionality isn't usable
> > > independently, it's still different type of code that's affected. And the
> > > patches are each big enough to make that worthwhile for easier review.
> >
> > Thank you for the feedback, pushed.
>
> Thanks for pushing them!
>
> > > One thing that'd be nice to do once we have WAIT FOR is to make the common
> > > case of wait_for_catchup() use this facility, instead of polling...
> >
> > The draft patch for that is attached.  WAIT FOR doesn't handle all the
> > possible use cases of wait_for_catchup(), but I've added usage when
> > it's appropriate.
>
> Interesting, could this approach be extended to the flush and other
> modes as well? I might need to spend some time to understand it before
> I can provide a meaningful review.

I think we might end up extending WaitLSNType enum.  However, I hate
inHeap and heapNode arrays growing in WaitLSNProcInfo as they are
allocated per process.  I found that we could optimize WaitLSNProcInfo
struct turning them into simple variables because a single process can
wait only for a single LSN at a time.  Please, check the attached
patch.

------
Regards,
Alexander Korotkov
Supabase

  43. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-11-12T07:19:53Z

    Hi,

On Wed, Nov 5, 2025 at 5:51 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> Hi!
>
> On Mon, Nov 3, 2025 at 5:13 PM Andres Freund <andres@anarazel.de> wrote:
> > On 2025-11-03 16:06:58 +0100, Álvaro Herrera wrote:
> > > On 2025-Nov-03, Alexander Korotkov wrote:
> > >
> > > > I'd like to give this subject another chance for pg19.  I'm going to
> > > > push this if no objections.
> > >
> > > Sure.  I don't understand why patches 0002 and 0003 are separate though.
> >
> > FWIW, I appreciate such splits. Even if the functionality isn't usable
> > independently, it's still different type of code that's affected. And the
> > patches are each big enough to make that worthwhile for easier review.
>
> Thank you for the feedback, pushed.
>
> > One thing that'd be nice to do once we have WAIT FOR is to make the common
> > case of wait_for_catchup() use this facility, instead of polling...
>
> The draft patch for that is attached.  WAIT FOR doesn't handle all the
> possible use cases of wait_for_catchup(), but I've added usage when
> it's appropriate.
>
> ------
> Regards,
> Alexander Korotkov
> Supabase

I tested the patch using make check-world, and it worked well. I also
made a few adjustments:

- Added an unconditional chomp($isrecovery) after querying
pg_is_in_recovery() to prevent newline mismatches when $target_lsn is
accidently defined.
- Added chomp($output) to normalize the result from WAIT FOR LSN
before comparison.

At the moment, the WAIT FOR LSN command supports only the replay mode.
If we intend to extend its functionality more broadly, one option is
to add a mode option or something similar. Are users expected to wait
for flush(or others) completion in such cases? If not, and the TAP
test is the only intended use, this approach might be a bit of an
overkill.

--
Best,
Xuneng

  44. Re: Implement waiting for wal lsn replay: reloaded

    Tomas Vondra <tomas@vondra.me> — 2025-11-13T20:32:31Z

    On 11/5/25 10:51, Alexander Korotkov wrote:
> Hi!
> 
> On Mon, Nov 3, 2025 at 5:13 PM Andres Freund <andres@anarazel.de> wrote:
>> On 2025-11-03 16:06:58 +0100, Álvaro Herrera wrote:
>>> On 2025-Nov-03, Alexander Korotkov wrote:
>>>
>>>> I'd like to give this subject another chance for pg19.  I'm going to
>>>> push this if no objections.
>>>
>>> Sure.  I don't understand why patches 0002 and 0003 are separate though.
>>
>> FWIW, I appreciate such splits. Even if the functionality isn't usable
>> independently, it's still different type of code that's affected. And the
>> patches are each big enough to make that worthwhile for easier review.
> 
> Thank you for the feedback, pushed.
> 

Hi,

The new TAP test 049_wait_for_lsn.pl introduced by this commit, because
it takes a long time - about 65 seconds on my laptop. That's about 25%
of the whole src/test/recovery, more than any other test.

And most of the time there's nothing happening - these are the two log
messages showing the 60-second wait:

2025-11-13 21:12:39.949 CET checkpointer[562597] LOG:  checkpoint
complete: wrote 9 buffers (7.0%), wrote 3 SLRU buffers; 0 WAL file(s)
added, 0 removed, 2 recycled; write=0.906 s, sync=0.001 s, total=0.907
s; sync files=0, longest=0.000 s, average=0.000 s; distance=32768 kB,
estimate=32768 kB; lsn=0/040000B8, redo lsn=0/04000060

2025-11-13 21:13:38.994 CET client backend[562727] 049_wait_for_lsn.pl
ERROR:  recovery is not in progress

So there's a checkpoint, 60 seconds of nothing, and then a failure. I
haven't looked into why it waits for 1 minute exactly, but adding 60
seconds to check-world is somewhat annoying.

While at it, I noticed a couple comments refer to WaitForLSNReplay, but
but I think that got renamed simply to WaitForLSN.


regards

-- 
Tomas Vondra

  45. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-11-14T01:49:51Z

    Hi Tomas,

On Fri, Nov 14, 2025 at 4:32 AM Tomas Vondra <tomas@vondra.me> wrote:
>
> On 11/5/25 10:51, Alexander Korotkov wrote:
> > Hi!
> >
> > On Mon, Nov 3, 2025 at 5:13 PM Andres Freund <andres@anarazel.de> wrote:
> >> On 2025-11-03 16:06:58 +0100, Álvaro Herrera wrote:
> >>> On 2025-Nov-03, Alexander Korotkov wrote:
> >>>
> >>>> I'd like to give this subject another chance for pg19.  I'm going to
> >>>> push this if no objections.
> >>>
> >>> Sure.  I don't understand why patches 0002 and 0003 are separate though.
> >>
> >> FWIW, I appreciate such splits. Even if the functionality isn't usable
> >> independently, it's still different type of code that's affected. And the
> >> patches are each big enough to make that worthwhile for easier review.
> >
> > Thank you for the feedback, pushed.
> >
>
> Hi,
>
> The new TAP test 049_wait_for_lsn.pl introduced by this commit, because
> it takes a long time - about 65 seconds on my laptop. That's about 25%
> of the whole src/test/recovery, more than any other test.
>
> And most of the time there's nothing happening - these are the two log
> messages showing the 60-second wait:
>
> 2025-11-13 21:12:39.949 CET checkpointer[562597] LOG:  checkpoint
> complete: wrote 9 buffers (7.0%), wrote 3 SLRU buffers; 0 WAL file(s)
> added, 0 removed, 2 recycled; write=0.906 s, sync=0.001 s, total=0.907
> s; sync files=0, longest=0.000 s, average=0.000 s; distance=32768 kB,
> estimate=32768 kB; lsn=0/040000B8, redo lsn=0/04000060
>
> 2025-11-13 21:13:38.994 CET client backend[562727] 049_wait_for_lsn.pl
> ERROR:  recovery is not in progress
>
> So there's a checkpoint, 60 seconds of nothing, and then a failure. I
> haven't looked into why it waits for 1 minute exactly, but adding 60
> seconds to check-world is somewhat annoying.

Thanks for looking into this!

I did a quick analysis for this prolonged waiting:

In WaitLSNWakeup() (xlogwait.c:267), the fast-path check incorrectly
handled InvalidXLogRecPtr:
/* Fast path check */
if (pg_atomic_read_u64(&waitLSNState->minWaitedLSN[i]) > currentLSN)
    return;  // Issue: Returns early when currentLSN = 0

When currentLSN = InvalidXLogRecPtr (0), meaning "wake all waiters",
the check compared:
- minWaitedLSN (e.g., 0x570CC048) > 0 → TRUE
- Result: function returned early without waking anyone

When It Happened
During standby promotion, xlog.c:6246 calls:

WaitLSNWakeup(WAIT_LSN_TYPE_REPLAY, InvalidXLogRecPtr);

This should wake all LSN waiters, but the bug prevented it. WAIT FOR
LSN commands could wait indefinitely. Test 049_wait_for_lsn.pl took 68
seconds instead of ~9 seconds.

if the above analysis is sound, the fix could be like:

Proposed fix:
Added a validity check before the comparison:
/*
 * Fast path check.  Skip if currentLSN is InvalidXLogRecPtr, which means
 * "wake all waiters" (e.g., during promotion when recovery ends).
 */
if (XLogRecPtrIsValid(currentLSN) &&
    pg_atomic_read_u64(&waitLSNState->minWaitedLSN[i]) > currentLSN)
    return;

Result:
Test time: 68s → 9s
WAIT FOR LSN exits immediately on promotion (62ms vs 60s)

> While at it, I noticed a couple comments refer to WaitForLSNReplay, but
> but I think that got renamed simply to WaitForLSN.

Please check the attached patch for replacing them.

-- 
Best,
Xuneng

  46. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-11-15T10:29:25Z

    Hi, Xuneng!

On Fri, Nov 14, 2025 at 3:50 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> On Fri, Nov 14, 2025 at 4:32 AM Tomas Vondra <tomas@vondra.me> wrote:
> >
> > On 11/5/25 10:51, Alexander Korotkov wrote:
> > > Hi!
> > >
> > > On Mon, Nov 3, 2025 at 5:13 PM Andres Freund <andres@anarazel.de> wrote:
> > >> On 2025-11-03 16:06:58 +0100, Álvaro Herrera wrote:
> > >>> On 2025-Nov-03, Alexander Korotkov wrote:
> > >>>
> > >>>> I'd like to give this subject another chance for pg19.  I'm going to
> > >>>> push this if no objections.
> > >>>
> > >>> Sure.  I don't understand why patches 0002 and 0003 are separate though.
> > >>
> > >> FWIW, I appreciate such splits. Even if the functionality isn't usable
> > >> independently, it's still different type of code that's affected. And the
> > >> patches are each big enough to make that worthwhile for easier review.
> > >
> > > Thank you for the feedback, pushed.
> > >
> >
> > Hi,
> >
> > The new TAP test 049_wait_for_lsn.pl introduced by this commit, because
> > it takes a long time - about 65 seconds on my laptop. That's about 25%
> > of the whole src/test/recovery, more than any other test.
> >
> > And most of the time there's nothing happening - these are the two log
> > messages showing the 60-second wait:
> >
> > 2025-11-13 21:12:39.949 CET checkpointer[562597] LOG:  checkpoint
> > complete: wrote 9 buffers (7.0%), wrote 3 SLRU buffers; 0 WAL file(s)
> > added, 0 removed, 2 recycled; write=0.906 s, sync=0.001 s, total=0.907
> > s; sync files=0, longest=0.000 s, average=0.000 s; distance=32768 kB,
> > estimate=32768 kB; lsn=0/040000B8, redo lsn=0/04000060
> >
> > 2025-11-13 21:13:38.994 CET client backend[562727] 049_wait_for_lsn.pl
> > ERROR:  recovery is not in progress
> >
> > So there's a checkpoint, 60 seconds of nothing, and then a failure. I
> > haven't looked into why it waits for 1 minute exactly, but adding 60
> > seconds to check-world is somewhat annoying.
>
> Thanks for looking into this!
>
> I did a quick analysis for this prolonged waiting:
>
> In WaitLSNWakeup() (xlogwait.c:267), the fast-path check incorrectly
> handled InvalidXLogRecPtr:
> /* Fast path check */
> if (pg_atomic_read_u64(&waitLSNState->minWaitedLSN[i]) > currentLSN)
>     return;  // Issue: Returns early when currentLSN = 0
>
> When currentLSN = InvalidXLogRecPtr (0), meaning "wake all waiters",
> the check compared:
> - minWaitedLSN (e.g., 0x570CC048) > 0 → TRUE
> - Result: function returned early without waking anyone
>
> When It Happened
> During standby promotion, xlog.c:6246 calls:
>
> WaitLSNWakeup(WAIT_LSN_TYPE_REPLAY, InvalidXLogRecPtr);
>
> This should wake all LSN waiters, but the bug prevented it. WAIT FOR
> LSN commands could wait indefinitely. Test 049_wait_for_lsn.pl took 68
> seconds instead of ~9 seconds.
>
> if the above analysis is sound, the fix could be like:
>
> Proposed fix:
> Added a validity check before the comparison:
> /*
>  * Fast path check.  Skip if currentLSN is InvalidXLogRecPtr, which means
>  * "wake all waiters" (e.g., during promotion when recovery ends).
>  */
> if (XLogRecPtrIsValid(currentLSN) &&
>     pg_atomic_read_u64(&waitLSNState->minWaitedLSN[i]) > currentLSN)
>     return;
>
> Result:
> Test time: 68s → 9s
> WAIT FOR LSN exits immediately on promotion (62ms vs 60s)
>
> > While at it, I noticed a couple comments refer to WaitForLSNReplay, but
> > but I think that got renamed simply to WaitForLSN.
>
> Please check the attached patch for replacing them.

Thank you so much for your patches!
Pushed with minor corrections.

------
Regards,
Alexander Korotkov
Supabase

  47. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-11-16T12:08:58Z

    On Sat, Nov 8, 2025 at 12:02 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Wed, Nov 5, 2025 at 4:03 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > On Wed, Nov 5, 2025 at 5:51 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > > On Mon, Nov 3, 2025 at 5:13 PM Andres Freund <andres@anarazel.de> wrote:
> > > > On 2025-11-03 16:06:58 +0100, Álvaro Herrera wrote:
> > > > > On 2025-Nov-03, Alexander Korotkov wrote:
> > > > >
> > > > > > I'd like to give this subject another chance for pg19.  I'm going to
> > > > > > push this if no objections.
> > > > >
> > > > > Sure.  I don't understand why patches 0002 and 0003 are separate though.
> > > >
> > > > FWIW, I appreciate such splits. Even if the functionality isn't usable
> > > > independently, it's still different type of code that's affected. And the
> > > > patches are each big enough to make that worthwhile for easier review.
> > >
> > > Thank you for the feedback, pushed.
> >
> > Thanks for pushing them!
> >
> > > > One thing that'd be nice to do once we have WAIT FOR is to make the common
> > > > case of wait_for_catchup() use this facility, instead of polling...
> > >
> > > The draft patch for that is attached.  WAIT FOR doesn't handle all the
> > > possible use cases of wait_for_catchup(), but I've added usage when
> > > it's appropriate.
> >
> > Interesting, could this approach be extended to the flush and other
> > modes as well? I might need to spend some time to understand it before
> > I can provide a meaningful review.
>
> I think we might end up extending WaitLSNType enum.  However, I hate
> inHeap and heapNode arrays growing in WaitLSNProcInfo as they are
> allocated per process.  I found that we could optimize WaitLSNProcInfo
> struct turning them into simple variables because a single process can
> wait only for a single LSN at a time.  Please, check the attached
> patch.

Here is the updated patch integrating minor corrections provided by
Xuneng Zhou off-list.  I'm going to push this if no objections.

------
Regards,
Alexander Korotkov
Supabase

  48. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-11-16T12:36:52Z

    On Wed, Nov 12, 2025 at 9:20 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> On Wed, Nov 5, 2025 at 5:51 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > On Mon, Nov 3, 2025 at 5:13 PM Andres Freund <andres@anarazel.de> wrote:
> > > On 2025-11-03 16:06:58 +0100, Álvaro Herrera wrote:
> > > > On 2025-Nov-03, Alexander Korotkov wrote:
> > > >
> > > > > I'd like to give this subject another chance for pg19.  I'm going to
> > > > > push this if no objections.
> > > >
> > > > Sure.  I don't understand why patches 0002 and 0003 are separate though.
> > >
> > > FWIW, I appreciate such splits. Even if the functionality isn't usable
> > > independently, it's still different type of code that's affected. And the
> > > patches are each big enough to make that worthwhile for easier review.
> >
> > Thank you for the feedback, pushed.
> >
> > > One thing that'd be nice to do once we have WAIT FOR is to make the common
> > > case of wait_for_catchup() use this facility, instead of polling...
> >
> > The draft patch for that is attached.  WAIT FOR doesn't handle all the
> > possible use cases of wait_for_catchup(), but I've added usage when
> > it's appropriate.
>
> I tested the patch using make check-world, and it worked well. I also
> made a few adjustments:
>
> - Added an unconditional chomp($isrecovery) after querying
> pg_is_in_recovery() to prevent newline mismatches when $target_lsn is
> accidently defined.
> - Added chomp($output) to normalize the result from WAIT FOR LSN
> before comparison.
>
> At the moment, the WAIT FOR LSN command supports only the replay mode.
> If we intend to extend its functionality more broadly, one option is
> to add a mode option or something similar. Are users expected to wait
> for flush(or others) completion in such cases? If not, and the TAP
> test is the only intended use, this approach might be a bit of an
> overkill.

I would say that adding mode parameter seems to be a pretty natural
extension of what we have at the moment.  I can imagine some
clustering solution can use it to wait for certain transaction to be
flushed at the replica (without delaying the commit at the primary).

------
Regards,
Alexander Korotkov
Supabase

  49. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-11-16T13:25:39Z

    Hi Alexander,

On Sat, Nov 15, 2025 at 6:29 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> Hi, Xuneng!
>
> On Fri, Nov 14, 2025 at 3:50 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > On Fri, Nov 14, 2025 at 4:32 AM Tomas Vondra <tomas@vondra.me> wrote:
> > >
> > > On 11/5/25 10:51, Alexander Korotkov wrote:
> > > > Hi!
> > > >
> > > > On Mon, Nov 3, 2025 at 5:13 PM Andres Freund <andres@anarazel.de> wrote:
> > > >> On 2025-11-03 16:06:58 +0100, Álvaro Herrera wrote:
> > > >>> On 2025-Nov-03, Alexander Korotkov wrote:
> > > >>>
> > > >>>> I'd like to give this subject another chance for pg19.  I'm going to
> > > >>>> push this if no objections.
> > > >>>
> > > >>> Sure.  I don't understand why patches 0002 and 0003 are separate though.
> > > >>
> > > >> FWIW, I appreciate such splits. Even if the functionality isn't usable
> > > >> independently, it's still different type of code that's affected. And the
> > > >> patches are each big enough to make that worthwhile for easier review.
> > > >
> > > > Thank you for the feedback, pushed.
> > > >
> > >
> > > Hi,
> > >
> > > The new TAP test 049_wait_for_lsn.pl introduced by this commit, because
> > > it takes a long time - about 65 seconds on my laptop. That's about 25%
> > > of the whole src/test/recovery, more than any other test.
> > >
> > > And most of the time there's nothing happening - these are the two log
> > > messages showing the 60-second wait:
> > >
> > > 2025-11-13 21:12:39.949 CET checkpointer[562597] LOG:  checkpoint
> > > complete: wrote 9 buffers (7.0%), wrote 3 SLRU buffers; 0 WAL file(s)
> > > added, 0 removed, 2 recycled; write=0.906 s, sync=0.001 s, total=0.907
> > > s; sync files=0, longest=0.000 s, average=0.000 s; distance=32768 kB,
> > > estimate=32768 kB; lsn=0/040000B8, redo lsn=0/04000060
> > >
> > > 2025-11-13 21:13:38.994 CET client backend[562727] 049_wait_for_lsn.pl
> > > ERROR:  recovery is not in progress
> > >
> > > So there's a checkpoint, 60 seconds of nothing, and then a failure. I
> > > haven't looked into why it waits for 1 minute exactly, but adding 60
> > > seconds to check-world is somewhat annoying.
> >
> > Thanks for looking into this!
> >
> > I did a quick analysis for this prolonged waiting:
> >
> > In WaitLSNWakeup() (xlogwait.c:267), the fast-path check incorrectly
> > handled InvalidXLogRecPtr:
> > /* Fast path check */
> > if (pg_atomic_read_u64(&waitLSNState->minWaitedLSN[i]) > currentLSN)
> >     return;  // Issue: Returns early when currentLSN = 0
> >
> > When currentLSN = InvalidXLogRecPtr (0), meaning "wake all waiters",
> > the check compared:
> > - minWaitedLSN (e.g., 0x570CC048) > 0 → TRUE
> > - Result: function returned early without waking anyone
> >
> > When It Happened
> > During standby promotion, xlog.c:6246 calls:
> >
> > WaitLSNWakeup(WAIT_LSN_TYPE_REPLAY, InvalidXLogRecPtr);
> >
> > This should wake all LSN waiters, but the bug prevented it. WAIT FOR
> > LSN commands could wait indefinitely. Test 049_wait_for_lsn.pl took 68
> > seconds instead of ~9 seconds.
> >
> > if the above analysis is sound, the fix could be like:
> >
> > Proposed fix:
> > Added a validity check before the comparison:
> > /*
> >  * Fast path check.  Skip if currentLSN is InvalidXLogRecPtr, which means
> >  * "wake all waiters" (e.g., during promotion when recovery ends).
> >  */
> > if (XLogRecPtrIsValid(currentLSN) &&
> >     pg_atomic_read_u64(&waitLSNState->minWaitedLSN[i]) > currentLSN)
> >     return;
> >
> > Result:
> > Test time: 68s → 9s
> > WAIT FOR LSN exits immediately on promotion (62ms vs 60s)
> >
> > > While at it, I noticed a couple comments refer to WaitForLSNReplay, but
> > > but I think that got renamed simply to WaitForLSN.
> >
> > Please check the attached patch for replacing them.
>
> Thank you so much for your patches!
> Pushed with minor corrections.

Thanks for pushing! It appears I should be running pgindent more regularly :).


--
Best,
Xuneng

  50. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-11-16T13:30:08Z

    Hi!

On Sun, Nov 16, 2025 at 8:09 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> On Sat, Nov 8, 2025 at 12:02 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > On Wed, Nov 5, 2025 at 4:03 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > On Wed, Nov 5, 2025 at 5:51 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > > > On Mon, Nov 3, 2025 at 5:13 PM Andres Freund <andres@anarazel.de> wrote:
> > > > > On 2025-11-03 16:06:58 +0100, Álvaro Herrera wrote:
> > > > > > On 2025-Nov-03, Alexander Korotkov wrote:
> > > > > >
> > > > > > > I'd like to give this subject another chance for pg19.  I'm going to
> > > > > > > push this if no objections.
> > > > > >
> > > > > > Sure.  I don't understand why patches 0002 and 0003 are separate though.
> > > > >
> > > > > FWIW, I appreciate such splits. Even if the functionality isn't usable
> > > > > independently, it's still different type of code that's affected. And the
> > > > > patches are each big enough to make that worthwhile for easier review.
> > > >
> > > > Thank you for the feedback, pushed.
> > >
> > > Thanks for pushing them!
> > >
> > > > > One thing that'd be nice to do once we have WAIT FOR is to make the common
> > > > > case of wait_for_catchup() use this facility, instead of polling...
> > > >
> > > > The draft patch for that is attached.  WAIT FOR doesn't handle all the
> > > > possible use cases of wait_for_catchup(), but I've added usage when
> > > > it's appropriate.
> > >
> > > Interesting, could this approach be extended to the flush and other
> > > modes as well? I might need to spend some time to understand it before
> > > I can provide a meaningful review.
> >
> > I think we might end up extending WaitLSNType enum.  However, I hate
> > inHeap and heapNode arrays growing in WaitLSNProcInfo as they are
> > allocated per process.  I found that we could optimize WaitLSNProcInfo
> > struct turning them into simple variables because a single process can
> > wait only for a single LSN at a time.  Please, check the attached
> > patch.
>
> Here is the updated patch integrating minor corrections provided by
> Xuneng Zhou off-list.  I'm going to push this if no objections.
>
> ------
> Regards,
> Alexander Korotkov
> Supabase

LGTM. Thanks.

-- 
Best,
Xuneng

  51. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-11-16T14:01:04Z

    Hi!

On Sun, Nov 16, 2025 at 8:37 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> On Wed, Nov 12, 2025 at 9:20 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > On Wed, Nov 5, 2025 at 5:51 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > > On Mon, Nov 3, 2025 at 5:13 PM Andres Freund <andres@anarazel.de> wrote:
> > > > On 2025-11-03 16:06:58 +0100, Álvaro Herrera wrote:
> > > > > On 2025-Nov-03, Alexander Korotkov wrote:
> > > > >
> > > > > > I'd like to give this subject another chance for pg19.  I'm going to
> > > > > > push this if no objections.
> > > > >
> > > > > Sure.  I don't understand why patches 0002 and 0003 are separate though.
> > > >
> > > > FWIW, I appreciate such splits. Even if the functionality isn't usable
> > > > independently, it's still different type of code that's affected. And the
> > > > patches are each big enough to make that worthwhile for easier review.
> > >
> > > Thank you for the feedback, pushed.
> > >
> > > > One thing that'd be nice to do once we have WAIT FOR is to make the common
> > > > case of wait_for_catchup() use this facility, instead of polling...
> > >
> > > The draft patch for that is attached.  WAIT FOR doesn't handle all the
> > > possible use cases of wait_for_catchup(), but I've added usage when
> > > it's appropriate.
> >
> > I tested the patch using make check-world, and it worked well. I also
> > made a few adjustments:
> >
> > - Added an unconditional chomp($isrecovery) after querying
> > pg_is_in_recovery() to prevent newline mismatches when $target_lsn is
> > accidently defined.
> > - Added chomp($output) to normalize the result from WAIT FOR LSN
> > before comparison.
> >
> > At the moment, the WAIT FOR LSN command supports only the replay mode.
> > If we intend to extend its functionality more broadly, one option is
> > to add a mode option or something similar. Are users expected to wait
> > for flush(or others) completion in such cases? If not, and the TAP
> > test is the only intended use, this approach might be a bit of an
> > overkill.
>
> I would say that adding mode parameter seems to be a pretty natural
> extension of what we have at the moment.  I can imagine some
> clustering solution can use it to wait for certain transaction to be
> flushed at the replica (without delaying the commit at the primary).
>
> ------
> Regards,
> Alexander Korotkov
> Supabase

Makes sense. I'll play with it and try to prepare a follow-up patch.

--
Best,
Xuneng

  52. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-11-16T15:20:25Z

    On Sun, Nov 16, 2025 at 3:25 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> On Sat, Nov 15, 2025 at 6:29 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > Thank you so much for your patches!
> > Pushed with minor corrections.
>
> Thanks for pushing! It appears I should be running pgindent more regularly :).

Thank you.  pgindent is not a problem for me, cause I anyway run it
every time before pushing a patch.  But yes, if you make it a habit to
run pgindent every time before publishing a patch, it would become
cleaner.

------
Regards,
Alexander Korotkov
Supabase

  53. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-11-20T12:43:06Z

    Hi Alexander, Hackers,

On Sun, Nov 16, 2025 at 10:01 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi!
>
> On Sun, Nov 16, 2025 at 8:37 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >
> > On Wed, Nov 12, 2025 at 9:20 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > On Wed, Nov 5, 2025 at 5:51 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > > > On Mon, Nov 3, 2025 at 5:13 PM Andres Freund <andres@anarazel.de> wrote:
> > > > > On 2025-11-03 16:06:58 +0100, Álvaro Herrera wrote:
> > > > > > On 2025-Nov-03, Alexander Korotkov wrote:
> > > > > >
> > > > > > > I'd like to give this subject another chance for pg19.  I'm going to
> > > > > > > push this if no objections.
> > > > > >
> > > > > > Sure.  I don't understand why patches 0002 and 0003 are separate though.
> > > > >
> > > > > FWIW, I appreciate such splits. Even if the functionality isn't usable
> > > > > independently, it's still different type of code that's affected. And the
> > > > > patches are each big enough to make that worthwhile for easier review.
> > > >
> > > > Thank you for the feedback, pushed.
> > > >
> > > > > One thing that'd be nice to do once we have WAIT FOR is to make the common
> > > > > case of wait_for_catchup() use this facility, instead of polling...
> > > >
> > > > The draft patch for that is attached.  WAIT FOR doesn't handle all the
> > > > possible use cases of wait_for_catchup(), but I've added usage when
> > > > it's appropriate.
> > >
> > > I tested the patch using make check-world, and it worked well. I also
> > > made a few adjustments:
> > >
> > > - Added an unconditional chomp($isrecovery) after querying
> > > pg_is_in_recovery() to prevent newline mismatches when $target_lsn is
> > > accidently defined.
> > > - Added chomp($output) to normalize the result from WAIT FOR LSN
> > > before comparison.
> > >
> > > At the moment, the WAIT FOR LSN command supports only the replay mode.
> > > If we intend to extend its functionality more broadly, one option is
> > > to add a mode option or something similar. Are users expected to wait
> > > for flush(or others) completion in such cases? If not, and the TAP
> > > test is the only intended use, this approach might be a bit of an
> > > overkill.
> >
> > I would say that adding mode parameter seems to be a pretty natural
> > extension of what we have at the moment.  I can imagine some
> > clustering solution can use it to wait for certain transaction to be
> > flushed at the replica (without delaying the commit at the primary).
> >
> > ------
> > Regards,
> > Alexander Korotkov
> > Supabase
>
> Makes sense. I'll play with it and try to prepare a follow-up patch.
>
> --
> Best,
> Xuneng

In terms of extending the functionality of the command, I see two
possible approaches here. One is to keep mode as a mandatory keyword,
and the other is to introduce it as an option in the WITH clause.

Syntax Option A: Mode in the WITH Clause

WAIT FOR LSN '0/12345' WITH (mode = 'replay');
WAIT FOR LSN '0/12345' WITH (mode = 'flush');
WAIT FOR LSN '0/12345' WITH (mode = 'write');

With this option, we can keep "replay" as the default mode. That means
existing TAP tests won’t need to be refactored unless they explicitly
want a different mode.

Syntax Option B: Mode as Part of the Main Command

WAIT FOR LSN '0/12345' MODE 'replay';
WAIT FOR LSN '0/12345' MODE 'flush';
WAIT FOR LSN '0/12345' MODE 'write';

Or a more concise variant using keywords:

WAIT FOR LSN '0/12345' REPLAY;
WAIT FOR LSN '0/12345' FLUSH;
WAIT FOR LSN '0/12345' WRITE;

This option produces a cleaner syntax if the intent is simply to wait
for a particular LSN type, without specifying additional options like
timeout or no_throw.

I don’t have a clear preference among them. I’d be interested to hear
what you or others think is the better direction.


--
Best,
Xuneng

  54. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-11-25T11:51:19Z

    Hi!

> > > > At the moment, the WAIT FOR LSN command supports only the replay mode.
> > > > If we intend to extend its functionality more broadly, one option is
> > > > to add a mode option or something similar. Are users expected to wait
> > > > for flush(or others) completion in such cases? If not, and the TAP
> > > > test is the only intended use, this approach might be a bit of an
> > > > overkill.
> > >
> > > I would say that adding mode parameter seems to be a pretty natural
> > > extension of what we have at the moment.  I can imagine some
> > > clustering solution can use it to wait for certain transaction to be
> > > flushed at the replica (without delaying the commit at the primary).
> > >
> > > ------
> > > Regards,
> > > Alexander Korotkov
> > > Supabase
> >
> > Makes sense. I'll play with it and try to prepare a follow-up patch.
> >
> > --
> > Best,
> > Xuneng
>
> In terms of extending the functionality of the command, I see two
> possible approaches here. One is to keep mode as a mandatory keyword,
> and the other is to introduce it as an option in the WITH clause.
>
> Syntax Option A: Mode in the WITH Clause
>
> WAIT FOR LSN '0/12345' WITH (mode = 'replay');
> WAIT FOR LSN '0/12345' WITH (mode = 'flush');
> WAIT FOR LSN '0/12345' WITH (mode = 'write');
>
> With this option, we can keep "replay" as the default mode. That means
> existing TAP tests won’t need to be refactored unless they explicitly
> want a different mode.
>
> Syntax Option B: Mode as Part of the Main Command
>
> WAIT FOR LSN '0/12345' MODE 'replay';
> WAIT FOR LSN '0/12345' MODE 'flush';
> WAIT FOR LSN '0/12345' MODE 'write';
>
> Or a more concise variant using keywords:
>
> WAIT FOR LSN '0/12345' REPLAY;
> WAIT FOR LSN '0/12345' FLUSH;
> WAIT FOR LSN '0/12345' WRITE;
>
> This option produces a cleaner syntax if the intent is simply to wait
> for a particular LSN type, without specifying additional options like
> timeout or no_throw.
>
> I don’t have a clear preference among them. I’d be interested to hear
> what you or others think is the better direction.
>

I've implemented a patch that adds MODE support to WAIT FOR LSN

The new grammar looks like:

——
WAIT FOR LSN '<lsn>' [MODE { REPLAY | WRITE | FLUSH }] [WITH (...)]
——

Two modes added: flush and write

Design decisions:

1. MODE as a separate keyword (not in WITH clause) - This follows the
pattern used by LOCK command. It also makes the common case more
concise.

2. REPLAY as the default - When MODE is not specified, it defaults to REPLAY.

3. Keywords rather than strings - Using `MODE WRITE` rather than `MODE 'write'`

The patch set includes:
-------
0001 - Extend xlogwait infrastructure with write and flush wait types

Adds WAIT_LSN_TYPE_WRITE and WAIT_LSN_TYPE_FLUSH to WaitLSNType enum,
along with corresponding wait events and pairing heaps. Introduces
GetCurrentLSNForWaitType() to retrieve the appropriate LSN based on
wait type, and adds wakeup calls in walreceiver for write/flush
events.

-------
0002 - Add pg_last_wal_write_lsn() SQL function

Adds a new SQL function that returns the current WAL write position on
a standby using GetWalRcvWriteRecPtr(). This complements existing
pg_last_wal_receive_lsn() (flush) and pg_last_wal_replay_lsn()
functions, enabling verification of WAIT FOR LSN MODE WRITE in TAP
tests.

-------
0003 - Add MODE parameter to WAIT FOR LSN command

Extends the parser and executor to support the optional MODE
parameter. Updates documentation with new syntax and mode
descriptions. Adds TAP tests covering all three modes including
mixed-mode concurrent waiters.

-------
0004 - Add tab completion for WAIT FOR LSN MODE parameter

Adds psql tab completion support: completes MODE after LSN value,
completes REPLAY/WRITE/FLUSH after MODE keyword, and completes WITH
after mode selection.

-------
0005 - Use WAIT FOR LSN in PostgreSQL::Test::Cluster::wait_for_catchup()

Replaces polling-based wait_for_catchup() with WAIT FOR LSN when the
target is a standby in recovery, improving test efficiency by avoiding
repeated queries.

The WRITE and FLUSH modes enable scenarios where applications need to
ensure WAL has been received or persisted on the standby without
waiting for replay to complete.

Feedback welcome.


--
Best,
Xuneng

  55. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-12-01T04:33:27Z

    Hi hackers,

On Tue, Nov 25, 2025 at 7:51 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi!
>
> > > > > At the moment, the WAIT FOR LSN command supports only the replay mode.
> > > > > If we intend to extend its functionality more broadly, one option is
> > > > > to add a mode option or something similar. Are users expected to wait
> > > > > for flush(or others) completion in such cases? If not, and the TAP
> > > > > test is the only intended use, this approach might be a bit of an
> > > > > overkill.
> > > >
> > > > I would say that adding mode parameter seems to be a pretty natural
> > > > extension of what we have at the moment.  I can imagine some
> > > > clustering solution can use it to wait for certain transaction to be
> > > > flushed at the replica (without delaying the commit at the primary).
> > > >
> > > > ------
> > > > Regards,
> > > > Alexander Korotkov
> > > > Supabase
> > >
> > > Makes sense. I'll play with it and try to prepare a follow-up patch.
> > >
> > > --
> > > Best,
> > > Xuneng
> >
> > In terms of extending the functionality of the command, I see two
> > possible approaches here. One is to keep mode as a mandatory keyword,
> > and the other is to introduce it as an option in the WITH clause.
> >
> > Syntax Option A: Mode in the WITH Clause
> >
> > WAIT FOR LSN '0/12345' WITH (mode = 'replay');
> > WAIT FOR LSN '0/12345' WITH (mode = 'flush');
> > WAIT FOR LSN '0/12345' WITH (mode = 'write');
> >
> > With this option, we can keep "replay" as the default mode. That means
> > existing TAP tests won’t need to be refactored unless they explicitly
> > want a different mode.
> >
> > Syntax Option B: Mode as Part of the Main Command
> >
> > WAIT FOR LSN '0/12345' MODE 'replay';
> > WAIT FOR LSN '0/12345' MODE 'flush';
> > WAIT FOR LSN '0/12345' MODE 'write';
> >
> > Or a more concise variant using keywords:
> >
> > WAIT FOR LSN '0/12345' REPLAY;
> > WAIT FOR LSN '0/12345' FLUSH;
> > WAIT FOR LSN '0/12345' WRITE;
> >
> > This option produces a cleaner syntax if the intent is simply to wait
> > for a particular LSN type, without specifying additional options like
> > timeout or no_throw.
> >
> > I don’t have a clear preference among them. I’d be interested to hear
> > what you or others think is the better direction.
> >
>
> I've implemented a patch that adds MODE support to WAIT FOR LSN
>
> The new grammar looks like:
>
> ——
> WAIT FOR LSN '<lsn>' [MODE { REPLAY | WRITE | FLUSH }] [WITH (...)]
> ——
>
> Two modes added: flush and write
>
> Design decisions:
>
> 1. MODE as a separate keyword (not in WITH clause) - This follows the
> pattern used by LOCK command. It also makes the common case more
> concise.
>
> 2. REPLAY as the default - When MODE is not specified, it defaults to REPLAY.
>
> 3. Keywords rather than strings - Using `MODE WRITE` rather than `MODE 'write'`
>
> The patch set includes:
> -------
> 0001 - Extend xlogwait infrastructure with write and flush wait types
>
> Adds WAIT_LSN_TYPE_WRITE and WAIT_LSN_TYPE_FLUSH to WaitLSNType enum,
> along with corresponding wait events and pairing heaps. Introduces
> GetCurrentLSNForWaitType() to retrieve the appropriate LSN based on
> wait type, and adds wakeup calls in walreceiver for write/flush
> events.
>
> -------
> 0002 - Add pg_last_wal_write_lsn() SQL function
>
> Adds a new SQL function that returns the current WAL write position on
> a standby using GetWalRcvWriteRecPtr(). This complements existing
> pg_last_wal_receive_lsn() (flush) and pg_last_wal_replay_lsn()
> functions, enabling verification of WAIT FOR LSN MODE WRITE in TAP
> tests.
>
> -------
> 0003 - Add MODE parameter to WAIT FOR LSN command
>
> Extends the parser and executor to support the optional MODE
> parameter. Updates documentation with new syntax and mode
> descriptions. Adds TAP tests covering all three modes including
> mixed-mode concurrent waiters.
>
> -------
> 0004 - Add tab completion for WAIT FOR LSN MODE parameter
>
> Adds psql tab completion support: completes MODE after LSN value,
> completes REPLAY/WRITE/FLUSH after MODE keyword, and completes WITH
> after mode selection.
>
> -------
> 0005 - Use WAIT FOR LSN in PostgreSQL::Test::Cluster::wait_for_catchup()
>
> Replaces polling-based wait_for_catchup() with WAIT FOR LSN when the
> target is a standby in recovery, improving test efficiency by avoiding
> repeated queries.
>
> The WRITE and FLUSH modes enable scenarios where applications need to
> ensure WAL has been received or persisted on the standby without
> waiting for replay to complete.
>
> Feedback welcome.
>

Here is the updated v2 patch set. Most of the updates are in patch 3.

Changes from v1:

Patch 1 (Extend wait types in xlogwait infra)
- Renamed enum values for consistency (WAIT_LSN_TYPE_REPLAY →
WAIT_LSN_TYPE_REPLAY_STANDBY, etc.)

Patch 2 (pg_last_wal_write_lsn):
- Clarified documentation and comment
- Improved pg_proc.dat description

Patch 3 (MODE parameter):
- Replaced direct cast with explicit switch statement for WaitLSNMode
→ WaitLSNType conversion
- Improved FLUSH/WRITE mode documentation with verification function references
- TAP tests (7b, 7c, 7d): Added walreceiver control for concurrency,
explicit blocking verification via poll_query_until, and log-based
completion verification via wait_for_log
- Fix the timing issue in wait for all three sessions to get the
errors after promotion of tap test 8.

-- 
Best,
Xuneng

  56. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-12-02T03:08:01Z

    Hi,

On Mon, Dec 1, 2025 at 12:33 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi hackers,
>
> On Tue, Nov 25, 2025 at 7:51 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi!
> >
> > > > > > At the moment, the WAIT FOR LSN command supports only the replay mode.
> > > > > > If we intend to extend its functionality more broadly, one option is
> > > > > > to add a mode option or something similar. Are users expected to wait
> > > > > > for flush(or others) completion in such cases? If not, and the TAP
> > > > > > test is the only intended use, this approach might be a bit of an
> > > > > > overkill.
> > > > >
> > > > > I would say that adding mode parameter seems to be a pretty natural
> > > > > extension of what we have at the moment.  I can imagine some
> > > > > clustering solution can use it to wait for certain transaction to be
> > > > > flushed at the replica (without delaying the commit at the primary).
> > > > >
> > > > > ------
> > > > > Regards,
> > > > > Alexander Korotkov
> > > > > Supabase
> > > >
> > > > Makes sense. I'll play with it and try to prepare a follow-up patch.
> > > >
> > > > --
> > > > Best,
> > > > Xuneng
> > >
> > > In terms of extending the functionality of the command, I see two
> > > possible approaches here. One is to keep mode as a mandatory keyword,
> > > and the other is to introduce it as an option in the WITH clause.
> > >
> > > Syntax Option A: Mode in the WITH Clause
> > >
> > > WAIT FOR LSN '0/12345' WITH (mode = 'replay');
> > > WAIT FOR LSN '0/12345' WITH (mode = 'flush');
> > > WAIT FOR LSN '0/12345' WITH (mode = 'write');
> > >
> > > With this option, we can keep "replay" as the default mode. That means
> > > existing TAP tests won’t need to be refactored unless they explicitly
> > > want a different mode.
> > >
> > > Syntax Option B: Mode as Part of the Main Command
> > >
> > > WAIT FOR LSN '0/12345' MODE 'replay';
> > > WAIT FOR LSN '0/12345' MODE 'flush';
> > > WAIT FOR LSN '0/12345' MODE 'write';
> > >
> > > Or a more concise variant using keywords:
> > >
> > > WAIT FOR LSN '0/12345' REPLAY;
> > > WAIT FOR LSN '0/12345' FLUSH;
> > > WAIT FOR LSN '0/12345' WRITE;
> > >
> > > This option produces a cleaner syntax if the intent is simply to wait
> > > for a particular LSN type, without specifying additional options like
> > > timeout or no_throw.
> > >
> > > I don’t have a clear preference among them. I’d be interested to hear
> > > what you or others think is the better direction.
> > >
> >
> > I've implemented a patch that adds MODE support to WAIT FOR LSN
> >
> > The new grammar looks like:
> >
> > ——
> > WAIT FOR LSN '<lsn>' [MODE { REPLAY | WRITE | FLUSH }] [WITH (...)]
> > ——
> >
> > Two modes added: flush and write
> >
> > Design decisions:
> >
> > 1. MODE as a separate keyword (not in WITH clause) - This follows the
> > pattern used by LOCK command. It also makes the common case more
> > concise.
> >
> > 2. REPLAY as the default - When MODE is not specified, it defaults to REPLAY.
> >
> > 3. Keywords rather than strings - Using `MODE WRITE` rather than `MODE 'write'`
> >
> > The patch set includes:
> > -------
> > 0001 - Extend xlogwait infrastructure with write and flush wait types
> >
> > Adds WAIT_LSN_TYPE_WRITE and WAIT_LSN_TYPE_FLUSH to WaitLSNType enum,
> > along with corresponding wait events and pairing heaps. Introduces
> > GetCurrentLSNForWaitType() to retrieve the appropriate LSN based on
> > wait type, and adds wakeup calls in walreceiver for write/flush
> > events.
> >
> > -------
> > 0002 - Add pg_last_wal_write_lsn() SQL function
> >
> > Adds a new SQL function that returns the current WAL write position on
> > a standby using GetWalRcvWriteRecPtr(). This complements existing
> > pg_last_wal_receive_lsn() (flush) and pg_last_wal_replay_lsn()
> > functions, enabling verification of WAIT FOR LSN MODE WRITE in TAP
> > tests.
> >
> > -------
> > 0003 - Add MODE parameter to WAIT FOR LSN command
> >
> > Extends the parser and executor to support the optional MODE
> > parameter. Updates documentation with new syntax and mode
> > descriptions. Adds TAP tests covering all three modes including
> > mixed-mode concurrent waiters.
> >
> > -------
> > 0004 - Add tab completion for WAIT FOR LSN MODE parameter
> >
> > Adds psql tab completion support: completes MODE after LSN value,
> > completes REPLAY/WRITE/FLUSH after MODE keyword, and completes WITH
> > after mode selection.
> >
> > -------
> > 0005 - Use WAIT FOR LSN in PostgreSQL::Test::Cluster::wait_for_catchup()
> >
> > Replaces polling-based wait_for_catchup() with WAIT FOR LSN when the
> > target is a standby in recovery, improving test efficiency by avoiding
> > repeated queries.
> >
> > The WRITE and FLUSH modes enable scenarios where applications need to
> > ensure WAL has been received or persisted on the standby without
> > waiting for replay to complete.
> >
> > Feedback welcome.
> >
>
> Here is the updated v2 patch set. Most of the updates are in patch 3.
>
> Changes from v1:
>
> Patch 1 (Extend wait types in xlogwait infra)
> - Renamed enum values for consistency (WAIT_LSN_TYPE_REPLAY →
> WAIT_LSN_TYPE_REPLAY_STANDBY, etc.)
>
> Patch 2 (pg_last_wal_write_lsn):
> - Clarified documentation and comment
> - Improved pg_proc.dat description
>
> Patch 3 (MODE parameter):
> - Replaced direct cast with explicit switch statement for WaitLSNMode
> → WaitLSNType conversion
> - Improved FLUSH/WRITE mode documentation with verification function references
> - TAP tests (7b, 7c, 7d): Added walreceiver control for concurrency,
> explicit blocking verification via poll_query_until, and log-based
> completion verification via wait_for_log
> - Fix the timing issue in wait for all three sessions to get the
> errors after promotion of tap test 8.
>
> --
> Best,
> Xuneng

Here is the updated v3. The changes are made to patch 3:

- Refactor duplicated TAP test code by extracting helper routines for
starting and stopping walreceiver.
- Increase the number of concurrent WRITE and FLUSH waiters in tests
7b and 7c from three to five, matching the number in test 7a.

-- 
Best,
Xuneng

  57. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-12-02T10:10:02Z

    Hi,

On Tue, Dec 2, 2025 at 11:08 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> On Mon, Dec 1, 2025 at 12:33 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi hackers,
> >
> > On Tue, Nov 25, 2025 at 7:51 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > >
> > > Hi!
> > >
> > > > > > > At the moment, the WAIT FOR LSN command supports only the replay mode.
> > > > > > > If we intend to extend its functionality more broadly, one option is
> > > > > > > to add a mode option or something similar. Are users expected to wait
> > > > > > > for flush(or others) completion in such cases? If not, and the TAP
> > > > > > > test is the only intended use, this approach might be a bit of an
> > > > > > > overkill.
> > > > > >
> > > > > > I would say that adding mode parameter seems to be a pretty natural
> > > > > > extension of what we have at the moment.  I can imagine some
> > > > > > clustering solution can use it to wait for certain transaction to be
> > > > > > flushed at the replica (without delaying the commit at the primary).
> > > > > >
> > > > > > ------
> > > > > > Regards,
> > > > > > Alexander Korotkov
> > > > > > Supabase
> > > > >
> > > > > Makes sense. I'll play with it and try to prepare a follow-up patch.
> > > > >
> > > > > --
> > > > > Best,
> > > > > Xuneng
> > > >
> > > > In terms of extending the functionality of the command, I see two
> > > > possible approaches here. One is to keep mode as a mandatory keyword,
> > > > and the other is to introduce it as an option in the WITH clause.
> > > >
> > > > Syntax Option A: Mode in the WITH Clause
> > > >
> > > > WAIT FOR LSN '0/12345' WITH (mode = 'replay');
> > > > WAIT FOR LSN '0/12345' WITH (mode = 'flush');
> > > > WAIT FOR LSN '0/12345' WITH (mode = 'write');
> > > >
> > > > With this option, we can keep "replay" as the default mode. That means
> > > > existing TAP tests won’t need to be refactored unless they explicitly
> > > > want a different mode.
> > > >
> > > > Syntax Option B: Mode as Part of the Main Command
> > > >
> > > > WAIT FOR LSN '0/12345' MODE 'replay';
> > > > WAIT FOR LSN '0/12345' MODE 'flush';
> > > > WAIT FOR LSN '0/12345' MODE 'write';
> > > >
> > > > Or a more concise variant using keywords:
> > > >
> > > > WAIT FOR LSN '0/12345' REPLAY;
> > > > WAIT FOR LSN '0/12345' FLUSH;
> > > > WAIT FOR LSN '0/12345' WRITE;
> > > >
> > > > This option produces a cleaner syntax if the intent is simply to wait
> > > > for a particular LSN type, without specifying additional options like
> > > > timeout or no_throw.
> > > >
> > > > I don’t have a clear preference among them. I’d be interested to hear
> > > > what you or others think is the better direction.
> > > >
> > >
> > > I've implemented a patch that adds MODE support to WAIT FOR LSN
> > >
> > > The new grammar looks like:
> > >
> > > ——
> > > WAIT FOR LSN '<lsn>' [MODE { REPLAY | WRITE | FLUSH }] [WITH (...)]
> > > ——
> > >
> > > Two modes added: flush and write
> > >
> > > Design decisions:
> > >
> > > 1. MODE as a separate keyword (not in WITH clause) - This follows the
> > > pattern used by LOCK command. It also makes the common case more
> > > concise.
> > >
> > > 2. REPLAY as the default - When MODE is not specified, it defaults to REPLAY.
> > >
> > > 3. Keywords rather than strings - Using `MODE WRITE` rather than `MODE 'write'`
> > >
> > > The patch set includes:
> > > -------
> > > 0001 - Extend xlogwait infrastructure with write and flush wait types
> > >
> > > Adds WAIT_LSN_TYPE_WRITE and WAIT_LSN_TYPE_FLUSH to WaitLSNType enum,
> > > along with corresponding wait events and pairing heaps. Introduces
> > > GetCurrentLSNForWaitType() to retrieve the appropriate LSN based on
> > > wait type, and adds wakeup calls in walreceiver for write/flush
> > > events.
> > >
> > > -------
> > > 0002 - Add pg_last_wal_write_lsn() SQL function
> > >
> > > Adds a new SQL function that returns the current WAL write position on
> > > a standby using GetWalRcvWriteRecPtr(). This complements existing
> > > pg_last_wal_receive_lsn() (flush) and pg_last_wal_replay_lsn()
> > > functions, enabling verification of WAIT FOR LSN MODE WRITE in TAP
> > > tests.
> > >
> > > -------
> > > 0003 - Add MODE parameter to WAIT FOR LSN command
> > >
> > > Extends the parser and executor to support the optional MODE
> > > parameter. Updates documentation with new syntax and mode
> > > descriptions. Adds TAP tests covering all three modes including
> > > mixed-mode concurrent waiters.
> > >
> > > -------
> > > 0004 - Add tab completion for WAIT FOR LSN MODE parameter
> > >
> > > Adds psql tab completion support: completes MODE after LSN value,
> > > completes REPLAY/WRITE/FLUSH after MODE keyword, and completes WITH
> > > after mode selection.
> > >
> > > -------
> > > 0005 - Use WAIT FOR LSN in PostgreSQL::Test::Cluster::wait_for_catchup()
> > >
> > > Replaces polling-based wait_for_catchup() with WAIT FOR LSN when the
> > > target is a standby in recovery, improving test efficiency by avoiding
> > > repeated queries.
> > >
> > > The WRITE and FLUSH modes enable scenarios where applications need to
> > > ensure WAL has been received or persisted on the standby without
> > > waiting for replay to complete.
> > >
> > > Feedback welcome.
> > >
> >
> > Here is the updated v2 patch set. Most of the updates are in patch 3.
> >
> > Changes from v1:
> >
> > Patch 1 (Extend wait types in xlogwait infra)
> > - Renamed enum values for consistency (WAIT_LSN_TYPE_REPLAY →
> > WAIT_LSN_TYPE_REPLAY_STANDBY, etc.)
> >
> > Patch 2 (pg_last_wal_write_lsn):
> > - Clarified documentation and comment
> > - Improved pg_proc.dat description
> >
> > Patch 3 (MODE parameter):
> > - Replaced direct cast with explicit switch statement for WaitLSNMode
> > → WaitLSNType conversion
> > - Improved FLUSH/WRITE mode documentation with verification function references
> > - TAP tests (7b, 7c, 7d): Added walreceiver control for concurrency,
> > explicit blocking verification via poll_query_until, and log-based
> > completion verification via wait_for_log
> > - Fix the timing issue in wait for all three sessions to get the
> > errors after promotion of tap test 8.
> >
> > --
> > Best,
> > Xuneng
>
> Here is the updated v3. The changes are made to patch 3:
>
> - Refactor duplicated TAP test code by extracting helper routines for
> starting and stopping walreceiver.
> - Increase the number of concurrent WRITE and FLUSH waiters in tests
> 7b and 7c from three to five, matching the number in test 7a.
>
> --
> Best,
> Xuneng

Just realized that patch 2 in prior emails could be dropped for
simplicity. Since the write LSN can be retrieved directly from
pg_stat_wal_receiver, the TAP test in patch 3 does not require a
separate SQL function for this purpose alone.


-- 
Best,
Xuneng

  58. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-12-16T03:28:51Z

    Hi,

On Tue, Dec 2, 2025 at 6:10 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> On Tue, Dec 2, 2025 at 11:08 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi,
> >
> > On Mon, Dec 1, 2025 at 12:33 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > >
> > > Hi hackers,
> > >
> > > On Tue, Nov 25, 2025 at 7:51 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > >
> > > > Hi!
> > > >
> > > > > > > > At the moment, the WAIT FOR LSN command supports only the replay mode.
> > > > > > > > If we intend to extend its functionality more broadly, one option is
> > > > > > > > to add a mode option or something similar. Are users expected to wait
> > > > > > > > for flush(or others) completion in such cases? If not, and the TAP
> > > > > > > > test is the only intended use, this approach might be a bit of an
> > > > > > > > overkill.
> > > > > > >
> > > > > > > I would say that adding mode parameter seems to be a pretty natural
> > > > > > > extension of what we have at the moment.  I can imagine some
> > > > > > > clustering solution can use it to wait for certain transaction to be
> > > > > > > flushed at the replica (without delaying the commit at the primary).
> > > > > > >
> > > > > > > ------
> > > > > > > Regards,
> > > > > > > Alexander Korotkov
> > > > > > > Supabase
> > > > > >
> > > > > > Makes sense. I'll play with it and try to prepare a follow-up patch.
> > > > > >
> > > > > > --
> > > > > > Best,
> > > > > > Xuneng
> > > > >
> > > > > In terms of extending the functionality of the command, I see two
> > > > > possible approaches here. One is to keep mode as a mandatory keyword,
> > > > > and the other is to introduce it as an option in the WITH clause.
> > > > >
> > > > > Syntax Option A: Mode in the WITH Clause
> > > > >
> > > > > WAIT FOR LSN '0/12345' WITH (mode = 'replay');
> > > > > WAIT FOR LSN '0/12345' WITH (mode = 'flush');
> > > > > WAIT FOR LSN '0/12345' WITH (mode = 'write');
> > > > >
> > > > > With this option, we can keep "replay" as the default mode. That means
> > > > > existing TAP tests won’t need to be refactored unless they explicitly
> > > > > want a different mode.
> > > > >
> > > > > Syntax Option B: Mode as Part of the Main Command
> > > > >
> > > > > WAIT FOR LSN '0/12345' MODE 'replay';
> > > > > WAIT FOR LSN '0/12345' MODE 'flush';
> > > > > WAIT FOR LSN '0/12345' MODE 'write';
> > > > >
> > > > > Or a more concise variant using keywords:
> > > > >
> > > > > WAIT FOR LSN '0/12345' REPLAY;
> > > > > WAIT FOR LSN '0/12345' FLUSH;
> > > > > WAIT FOR LSN '0/12345' WRITE;
> > > > >
> > > > > This option produces a cleaner syntax if the intent is simply to wait
> > > > > for a particular LSN type, without specifying additional options like
> > > > > timeout or no_throw.
> > > > >
> > > > > I don’t have a clear preference among them. I’d be interested to hear
> > > > > what you or others think is the better direction.
> > > > >
> > > >
> > > > I've implemented a patch that adds MODE support to WAIT FOR LSN
> > > >
> > > > The new grammar looks like:
> > > >
> > > > ——
> > > > WAIT FOR LSN '<lsn>' [MODE { REPLAY | WRITE | FLUSH }] [WITH (...)]
> > > > ——
> > > >
> > > > Two modes added: flush and write
> > > >
> > > > Design decisions:
> > > >
> > > > 1. MODE as a separate keyword (not in WITH clause) - This follows the
> > > > pattern used by LOCK command. It also makes the common case more
> > > > concise.
> > > >
> > > > 2. REPLAY as the default - When MODE is not specified, it defaults to REPLAY.
> > > >
> > > > 3. Keywords rather than strings - Using `MODE WRITE` rather than `MODE 'write'`
> > > >
> > > > The patch set includes:
> > > > -------
> > > > 0001 - Extend xlogwait infrastructure with write and flush wait types
> > > >
> > > > Adds WAIT_LSN_TYPE_WRITE and WAIT_LSN_TYPE_FLUSH to WaitLSNType enum,
> > > > along with corresponding wait events and pairing heaps. Introduces
> > > > GetCurrentLSNForWaitType() to retrieve the appropriate LSN based on
> > > > wait type, and adds wakeup calls in walreceiver for write/flush
> > > > events.
> > > >
> > > > -------
> > > > 0002 - Add pg_last_wal_write_lsn() SQL function
> > > >
> > > > Adds a new SQL function that returns the current WAL write position on
> > > > a standby using GetWalRcvWriteRecPtr(). This complements existing
> > > > pg_last_wal_receive_lsn() (flush) and pg_last_wal_replay_lsn()
> > > > functions, enabling verification of WAIT FOR LSN MODE WRITE in TAP
> > > > tests.
> > > >
> > > > -------
> > > > 0003 - Add MODE parameter to WAIT FOR LSN command
> > > >
> > > > Extends the parser and executor to support the optional MODE
> > > > parameter. Updates documentation with new syntax and mode
> > > > descriptions. Adds TAP tests covering all three modes including
> > > > mixed-mode concurrent waiters.
> > > >
> > > > -------
> > > > 0004 - Add tab completion for WAIT FOR LSN MODE parameter
> > > >
> > > > Adds psql tab completion support: completes MODE after LSN value,
> > > > completes REPLAY/WRITE/FLUSH after MODE keyword, and completes WITH
> > > > after mode selection.
> > > >
> > > > -------
> > > > 0005 - Use WAIT FOR LSN in PostgreSQL::Test::Cluster::wait_for_catchup()
> > > >
> > > > Replaces polling-based wait_for_catchup() with WAIT FOR LSN when the
> > > > target is a standby in recovery, improving test efficiency by avoiding
> > > > repeated queries.
> > > >
> > > > The WRITE and FLUSH modes enable scenarios where applications need to
> > > > ensure WAL has been received or persisted on the standby without
> > > > waiting for replay to complete.
> > > >
> > > > Feedback welcome.
> > > >
> > >
> > > Here is the updated v2 patch set. Most of the updates are in patch 3.
> > >
> > > Changes from v1:
> > >
> > > Patch 1 (Extend wait types in xlogwait infra)
> > > - Renamed enum values for consistency (WAIT_LSN_TYPE_REPLAY →
> > > WAIT_LSN_TYPE_REPLAY_STANDBY, etc.)
> > >
> > > Patch 2 (pg_last_wal_write_lsn):
> > > - Clarified documentation and comment
> > > - Improved pg_proc.dat description
> > >
> > > Patch 3 (MODE parameter):
> > > - Replaced direct cast with explicit switch statement for WaitLSNMode
> > > → WaitLSNType conversion
> > > - Improved FLUSH/WRITE mode documentation with verification function references
> > > - TAP tests (7b, 7c, 7d): Added walreceiver control for concurrency,
> > > explicit blocking verification via poll_query_until, and log-based
> > > completion verification via wait_for_log
> > > - Fix the timing issue in wait for all three sessions to get the
> > > errors after promotion of tap test 8.
> > >
> > > --
> > > Best,
> > > Xuneng
> >
> > Here is the updated v3. The changes are made to patch 3:
> >
> > - Refactor duplicated TAP test code by extracting helper routines for
> > starting and stopping walreceiver.
> > - Increase the number of concurrent WRITE and FLUSH waiters in tests
> > 7b and 7c from three to five, matching the number in test 7a.
> >
> > --
> > Best,
> > Xuneng
>
> Just realized that patch 2 in prior emails could be dropped for
> simplicity. Since the write LSN can be retrieved directly from
> pg_stat_wal_receiver, the TAP test in patch 3 does not require a
> separate SQL function for this purpose alone.
>

Just rebase with minor changes to the wait-lsn types.

-- 
Best,
Xuneng

  59. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-12-16T04:46:27Z

    Hi,

On Tue, Dec 16, 2025 at 11:28 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> On Tue, Dec 2, 2025 at 6:10 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi,
> >
> > On Tue, Dec 2, 2025 at 11:08 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Dec 1, 2025 at 12:33 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > >
> > > > Hi hackers,
> > > >
> > > > On Tue, Nov 25, 2025 at 7:51 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > > >
> > > > > Hi!
> > > > >
> > > > > > > > > At the moment, the WAIT FOR LSN command supports only the replay mode.
> > > > > > > > > If we intend to extend its functionality more broadly, one option is
> > > > > > > > > to add a mode option or something similar. Are users expected to wait
> > > > > > > > > for flush(or others) completion in such cases? If not, and the TAP
> > > > > > > > > test is the only intended use, this approach might be a bit of an
> > > > > > > > > overkill.
> > > > > > > >
> > > > > > > > I would say that adding mode parameter seems to be a pretty natural
> > > > > > > > extension of what we have at the moment.  I can imagine some
> > > > > > > > clustering solution can use it to wait for certain transaction to be
> > > > > > > > flushed at the replica (without delaying the commit at the primary).
> > > > > > > >
> > > > > > > > ------
> > > > > > > > Regards,
> > > > > > > > Alexander Korotkov
> > > > > > > > Supabase
> > > > > > >
> > > > > > > Makes sense. I'll play with it and try to prepare a follow-up patch.
> > > > > > >
> > > > > > > --
> > > > > > > Best,
> > > > > > > Xuneng
> > > > > >
> > > > > > In terms of extending the functionality of the command, I see two
> > > > > > possible approaches here. One is to keep mode as a mandatory keyword,
> > > > > > and the other is to introduce it as an option in the WITH clause.
> > > > > >
> > > > > > Syntax Option A: Mode in the WITH Clause
> > > > > >
> > > > > > WAIT FOR LSN '0/12345' WITH (mode = 'replay');
> > > > > > WAIT FOR LSN '0/12345' WITH (mode = 'flush');
> > > > > > WAIT FOR LSN '0/12345' WITH (mode = 'write');
> > > > > >
> > > > > > With this option, we can keep "replay" as the default mode. That means
> > > > > > existing TAP tests won’t need to be refactored unless they explicitly
> > > > > > want a different mode.
> > > > > >
> > > > > > Syntax Option B: Mode as Part of the Main Command
> > > > > >
> > > > > > WAIT FOR LSN '0/12345' MODE 'replay';
> > > > > > WAIT FOR LSN '0/12345' MODE 'flush';
> > > > > > WAIT FOR LSN '0/12345' MODE 'write';
> > > > > >
> > > > > > Or a more concise variant using keywords:
> > > > > >
> > > > > > WAIT FOR LSN '0/12345' REPLAY;
> > > > > > WAIT FOR LSN '0/12345' FLUSH;
> > > > > > WAIT FOR LSN '0/12345' WRITE;
> > > > > >
> > > > > > This option produces a cleaner syntax if the intent is simply to wait
> > > > > > for a particular LSN type, without specifying additional options like
> > > > > > timeout or no_throw.
> > > > > >
> > > > > > I don’t have a clear preference among them. I’d be interested to hear
> > > > > > what you or others think is the better direction.
> > > > > >
> > > > >
> > > > > I've implemented a patch that adds MODE support to WAIT FOR LSN
> > > > >
> > > > > The new grammar looks like:
> > > > >
> > > > > ——
> > > > > WAIT FOR LSN '<lsn>' [MODE { REPLAY | WRITE | FLUSH }] [WITH (...)]
> > > > > ——
> > > > >
> > > > > Two modes added: flush and write
> > > > >
> > > > > Design decisions:
> > > > >
> > > > > 1. MODE as a separate keyword (not in WITH clause) - This follows the
> > > > > pattern used by LOCK command. It also makes the common case more
> > > > > concise.
> > > > >
> > > > > 2. REPLAY as the default - When MODE is not specified, it defaults to REPLAY.
> > > > >
> > > > > 3. Keywords rather than strings - Using `MODE WRITE` rather than `MODE 'write'`
> > > > >
> > > > > The patch set includes:
> > > > > -------
> > > > > 0001 - Extend xlogwait infrastructure with write and flush wait types
> > > > >
> > > > > Adds WAIT_LSN_TYPE_WRITE and WAIT_LSN_TYPE_FLUSH to WaitLSNType enum,
> > > > > along with corresponding wait events and pairing heaps. Introduces
> > > > > GetCurrentLSNForWaitType() to retrieve the appropriate LSN based on
> > > > > wait type, and adds wakeup calls in walreceiver for write/flush
> > > > > events.
> > > > >
> > > > > -------
> > > > > 0002 - Add pg_last_wal_write_lsn() SQL function
> > > > >
> > > > > Adds a new SQL function that returns the current WAL write position on
> > > > > a standby using GetWalRcvWriteRecPtr(). This complements existing
> > > > > pg_last_wal_receive_lsn() (flush) and pg_last_wal_replay_lsn()
> > > > > functions, enabling verification of WAIT FOR LSN MODE WRITE in TAP
> > > > > tests.
> > > > >
> > > > > -------
> > > > > 0003 - Add MODE parameter to WAIT FOR LSN command
> > > > >
> > > > > Extends the parser and executor to support the optional MODE
> > > > > parameter. Updates documentation with new syntax and mode
> > > > > descriptions. Adds TAP tests covering all three modes including
> > > > > mixed-mode concurrent waiters.
> > > > >
> > > > > -------
> > > > > 0004 - Add tab completion for WAIT FOR LSN MODE parameter
> > > > >
> > > > > Adds psql tab completion support: completes MODE after LSN value,
> > > > > completes REPLAY/WRITE/FLUSH after MODE keyword, and completes WITH
> > > > > after mode selection.
> > > > >
> > > > > -------
> > > > > 0005 - Use WAIT FOR LSN in PostgreSQL::Test::Cluster::wait_for_catchup()
> > > > >
> > > > > Replaces polling-based wait_for_catchup() with WAIT FOR LSN when the
> > > > > target is a standby in recovery, improving test efficiency by avoiding
> > > > > repeated queries.
> > > > >
> > > > > The WRITE and FLUSH modes enable scenarios where applications need to
> > > > > ensure WAL has been received or persisted on the standby without
> > > > > waiting for replay to complete.
> > > > >
> > > > > Feedback welcome.
> > > > >
> > > >
> > > > Here is the updated v2 patch set. Most of the updates are in patch 3.
> > > >
> > > > Changes from v1:
> > > >
> > > > Patch 1 (Extend wait types in xlogwait infra)
> > > > - Renamed enum values for consistency (WAIT_LSN_TYPE_REPLAY →
> > > > WAIT_LSN_TYPE_REPLAY_STANDBY, etc.)
> > > >
> > > > Patch 2 (pg_last_wal_write_lsn):
> > > > - Clarified documentation and comment
> > > > - Improved pg_proc.dat description
> > > >
> > > > Patch 3 (MODE parameter):
> > > > - Replaced direct cast with explicit switch statement for WaitLSNMode
> > > > → WaitLSNType conversion
> > > > - Improved FLUSH/WRITE mode documentation with verification function references
> > > > - TAP tests (7b, 7c, 7d): Added walreceiver control for concurrency,
> > > > explicit blocking verification via poll_query_until, and log-based
> > > > completion verification via wait_for_log
> > > > - Fix the timing issue in wait for all three sessions to get the
> > > > errors after promotion of tap test 8.
> > > >
> > > > --
> > > > Best,
> > > > Xuneng
> > >
> > > Here is the updated v3. The changes are made to patch 3:
> > >
> > > - Refactor duplicated TAP test code by extracting helper routines for
> > > starting and stopping walreceiver.
> > > - Increase the number of concurrent WRITE and FLUSH waiters in tests
> > > 7b and 7c from three to five, matching the number in test 7a.
> > >
> > > --
> > > Best,
> > > Xuneng
> >
> > Just realized that patch 2 in prior emails could be dropped for
> > simplicity. Since the write LSN can be retrieved directly from
> > pg_stat_wal_receiver, the TAP test in patch 3 does not require a
> > separate SQL function for this purpose alone.
> >
>
> Just rebase with minor changes to the wait-lsn types.
>

Remove the erroneous WAIT_LSN_TYPE_COUNT case from the switch
statement in v5 patch 1.

-- 
Best,
Xuneng

  60. Re: Implement waiting for wal lsn replay: reloaded

    Chao Li <li.evan.chao@gmail.com> — 2025-12-16T05:48:37Z

    
> On Oct 4, 2025, at 09:35, Xuneng Zhou <xunengzhou@gmail.com> wrote:
> 
> Hi,
> 
> On Sun, Sep 28, 2025 at 5:02 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>> 
>> Hi,
>> 
>> On Fri, Sep 26, 2025 at 7:22 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>>> 
>>> Hi Álvaro,
>>> 
>>> Thanks for your review.
>>> 
>>> On Tue, Sep 16, 2025 at 4:24 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>>>> 
>>>> On 2025-Sep-15, Alexander Korotkov wrote:
>>>> 
>>>>>> It's LGTM. The same pattern is observed in VACUUM, EXPLAIN, and CREATE
>>>>>> PUBLICATION - all use minimal grammar rules that produce generic
>>>>>> option lists, with the actual interpretation done in their respective
>>>>>> implementation files. The moderate complexity in wait.c seems
>>>>>> acceptable.
>>>> 
>>>> Actually I find the code in ExecWaitStmt pretty unusual.  We tend to use
>>>> lists of DefElem (a name optionally followed by a value) instead of
>>>> individual scattered elements that must later be matched up.  Why not
>>>> use utility_option_list instead and then loop on the list of DefElems?
>>>> It'd be a lot simpler.
>>> 
>>> I took a look at commands like VACUUM and EXPLAIN and they do follow
>>> this pattern. v11 will make use of utility_option_list.
>>> 
>>>> Also, we've found that failing to surround the options by parens leads
>>>> to pain down the road, so maybe add that.  Given that the LSN seems to
>>>> be mandatory, maybe make it something like
>>>> 
>>>> WAIT FOR LSN 'xy/zzy' [ WITH ( utility_option_list ) ]
>>>> 
>>>> This requires that you make LSN a keyword, albeit unreserved.  Or you
>>>> could make it
>>>> WAIT FOR Ident [the rest]
>>>> and then ensure in C that the identifier matches the word LSN, such as
>>>> we do for "permissive" and "restrictive" in
>>>> RowSecurityDefaultPermissive.
>>> 
>>> Shall make LSN an unreserved keyword as well.
>> 
>> Here's the updated v11.  Many thanks Jian for off-list discussions and review.
> 
> v12 removed unused
> +WaitStmt
> +WaitStmtParam in pgindent/typedefs.list.
> 
> Best,
> Xuneng
> <v12-0001-Implement-WAIT-FOR-command.patch>

I just tried to review v12 but failed to “git am”. Can you please rebase the change?

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

  61. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-12-16T06:42:00Z

    Hi,

On Tue, Dec 16, 2025 at 1:49 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On Oct 4, 2025, at 09:35, Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi,
> >
> > On Sun, Sep 28, 2025 at 5:02 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >>
> >> Hi,
> >>
> >> On Fri, Sep 26, 2025 at 7:22 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >>>
> >>> Hi Álvaro,
> >>>
> >>> Thanks for your review.
> >>>
> >>> On Tue, Sep 16, 2025 at 4:24 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> >>>>
> >>>> On 2025-Sep-15, Alexander Korotkov wrote:
> >>>>
> >>>>>> It's LGTM. The same pattern is observed in VACUUM, EXPLAIN, and CREATE
> >>>>>> PUBLICATION - all use minimal grammar rules that produce generic
> >>>>>> option lists, with the actual interpretation done in their respective
> >>>>>> implementation files. The moderate complexity in wait.c seems
> >>>>>> acceptable.
> >>>>
> >>>> Actually I find the code in ExecWaitStmt pretty unusual.  We tend to use
> >>>> lists of DefElem (a name optionally followed by a value) instead of
> >>>> individual scattered elements that must later be matched up.  Why not
> >>>> use utility_option_list instead and then loop on the list of DefElems?
> >>>> It'd be a lot simpler.
> >>>
> >>> I took a look at commands like VACUUM and EXPLAIN and they do follow
> >>> this pattern. v11 will make use of utility_option_list.
> >>>
> >>>> Also, we've found that failing to surround the options by parens leads
> >>>> to pain down the road, so maybe add that.  Given that the LSN seems to
> >>>> be mandatory, maybe make it something like
> >>>>
> >>>> WAIT FOR LSN 'xy/zzy' [ WITH ( utility_option_list ) ]
> >>>>
> >>>> This requires that you make LSN a keyword, albeit unreserved.  Or you
> >>>> could make it
> >>>> WAIT FOR Ident [the rest]
> >>>> and then ensure in C that the identifier matches the word LSN, such as
> >>>> we do for "permissive" and "restrictive" in
> >>>> RowSecurityDefaultPermissive.
> >>>
> >>> Shall make LSN an unreserved keyword as well.
> >>
> >> Here's the updated v11.  Many thanks Jian for off-list discussions and review.
> >
> > v12 removed unused
> > +WaitStmt
> > +WaitStmtParam in pgindent/typedefs.list.
> >
> > Best,
> > Xuneng
> > <v12-0001-Implement-WAIT-FOR-command.patch>
>
> I just tried to review v12 but failed to “git am”. Can you please rebase the change?
>

Thanks for looking into this.

That series of patches implementing the WAIT FOR REPLAY command was
applied last month (8af3ae0d , 447aae13, 3b4e53a0, a1f7f91b) in its
version 20. The current v6 patch set [1] [2] primarily extends the
WAIT FOR functionality to support waiting for flush and write LSNs on
a replica by adding a MODE parameter [3]. This made me wonder whether
it would be more appropriate to start a new thread for the extension,
though it is still part of the same WAIT FOR command.

[1] https://commitfest.postgresql.org/patch/6265/
[2] https://www.postgresql.org/message-id/CABPTF7XKti620ZAOXPGuhSZxCKyaV_9stq7ruhnuxvshUxCeRQ@mail.gmail.com
[3] https://www.postgresql.org/message-id/CAPpHfdt4b0wBC4+Oopp_eFQnNjDvxwQLrQ1r4GMJfCY0XWP0dA@mail.gmail.com

-- 
Best,
Xuneng

  62. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-12-18T10:38:43Z

    Hi, Xuneng!

On Tue, Dec 16, 2025 at 6:46 AM Xuneng Zhou <xunengzhou@gmail.com> 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?

------
Regards,
Alexander Korotkov

  63. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-12-18T12:24:38Z

    Hi Alexander,

On Thu, Dec 18, 2025 at 6:38 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> Hi, Xuneng!
>
> On Tue, Dec 16, 2025 at 6:46 AM Xuneng Zhou <xunengzhou@gmail.com> 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.

-- 
Best,
Xuneng

  64. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-12-18T12:25:43Z

    On Thu, Dec 18, 2025 at 2:24 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> On Thu, Dec 18, 2025 at 6:38 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >
> > Hi, Xuneng!
> >
> > On Tue, Dec 16, 2025 at 6:46 AM Xuneng Zhou <xunengzhou@gmail.com> 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.

------
Regards,
Alexander Korotkov
Supabase

  65. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-12-19T02:49:46Z

    Hi,

On Thu, Dec 18, 2025 at 8:25 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> On Thu, Dec 18, 2025 at 2:24 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > On Thu, Dec 18, 2025 at 6:38 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > >
> > > Hi, Xuneng!
> > >
> > > On Tue, Dec 16, 2025 at 6:46 AM Xuneng Zhou <xunengzhou@gmail.com> 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

  66. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-12-20T21:09:43Z

    On Fri, Dec 19, 2025 at 4:50 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> On Thu, Dec 18, 2025 at 8:25 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >
> > On Thu, Dec 18, 2025 at 2:24 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > On Thu, Dec 18, 2025 at 6:38 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > > >
> > > > Hi, Xuneng!
> > > >
> > > > On Tue, Dec 16, 2025 at 6:46 AM Xuneng Zhou <xunengzhou@gmail.com> 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.

I see that we can't specify WAIT_LSN_TYPE_PRIMARY_FLUSH by setting
mode parameter.  Should we allow this?

If we allow specifying WAIT_LSN_TYPE_PRIMARY_FLUSH, should it be
separate mode value or the same with WAIT_LSN_TYPE_STANDBY_FLUSH?  In
principle, we could encode both as just 'flush' mode, and detect which
WaitLSNType to pick by checking if recovery is in progress.  However,
how should we then react to unreached flush location after standby
promotion (technically it could be still reached but on the different
timeline)?

------
Regards,
Alexander Korotkov
Supabase

  67. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-12-21T04:37:18Z

    Hi Alexander,

Thanks for your feedback!

> I see that we can't specify WAIT_LSN_TYPE_PRIMARY_FLUSH by setting
> mode parameter.  Should we allow this?

I think this constraint could be relaxed if needed. I was previously
unsure about the use cases.

> If we allow specifying WAIT_LSN_TYPE_PRIMARY_FLUSH, should it be
> separate mode value or the same with WAIT_LSN_TYPE_STANDBY_FLUSH?  In
> principle, we could encode both as just 'flush' mode, and detect which
> WaitLSNType to pick by checking if recovery is in progress.  However,
> how should we then react to unreached flush location after standby
> promotion (technically it could be still reached but on the different
> timeline)?
>

Technically, we can use 'flush' mode to specify WAIT FOR behavior in
both primary and replica. Currently, wait for commands error out if
promotion occurs since: either the requested LSN type does not exist
on the primary, or we do not yet have the infrastructure to support
continuing the wait. If we allow waiting for flush on the primary as a
user-visible command and the wake-up calls for flush in primary are
introduced, the question becomes whether we should still abort the
wait on promotion, or continue waiting—as you noted—given that the
target LSN might still be reached, albeit on a different timeline. The
question behind this might be: do users care and should be aware of
the state change of the server while waiting? If they do, then we
better stop the waiting and report the error. In this case, I am
inclined to to break the unified flush mode to something like
primary_flush/standby_flush mode and
WAIT_LSN_TYPE_PRIMARY_FLUSH/WAIT_LSN_TYPE_STANDBY_FLUSH respectively.

--
Best,
Xuneng

  68. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-12-22T07:56:59Z

    Hi,

On Sun, Dec 21, 2025 at 12:37 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi Alexander,
>
> Thanks for your feedback!
>
> > I see that we can't specify WAIT_LSN_TYPE_PRIMARY_FLUSH by setting
> > mode parameter.  Should we allow this?
>
> I think this constraint could be relaxed if needed. I was previously
> unsure about the use cases.

Flush mode on the primary seems useful when synchronous_commit is set
to off [1]. In that mode, a transaction in primary may return success
before its WAL is durably flushed to disk, trading durability for
lower latency. A “wait for primary flush” operation provides an
explicit durability barrier for cases where applications or tools
occasionally need stronger guarantees.

[1] https://postgresqlco.nf/doc/en/param/synchronous_commit/

> > If we allow specifying WAIT_LSN_TYPE_PRIMARY_FLUSH, should it be
> > separate mode value or the same with WAIT_LSN_TYPE_STANDBY_FLUSH?  In
> > principle, we could encode both as just 'flush' mode, and detect which
> > WaitLSNType to pick by checking if recovery is in progress.  However,
> > how should we then react to unreached flush location after standby
> > promotion (technically it could be still reached but on the different
> > timeline)?
> >
>
> Technically, we can use 'flush' mode to specify WAIT FOR behavior in
> both primary and replica. Currently, wait for commands error out if
> promotion occurs since: either the requested LSN type does not exist
> on the primary, or we do not yet have the infrastructure to support
> continuing the wait. If we allow waiting for flush on the primary as a
> user-visible command and the wake-up calls for flush in primary are
> introduced, the question becomes whether we should still abort the
> wait on promotion, or continue waiting—as you noted—given that the
> target LSN might still be reached, albeit on a different timeline. The
> question behind this might be: do users care and should be aware of
> the state change of the server while waiting? If they do, then we
> better stop the waiting and report the error. In this case, I am
> inclined to to break the unified flush mode to something like
> primary_flush/standby_flush mode and
> WAIT_LSN_TYPE_PRIMARY_FLUSH/WAIT_LSN_TYPE_STANDBY_FLUSH respectively.
>

After further consideration, it also seems reasonable to use a single,
unified flush mode that works on both primary and standby servers,
provided its semantics are clearly documented to avoid the potential
confusion on failure. I don’t have a strong preference between these
two and would be interested in your thoughts.

If a standby is promoted while a session is waiting, the command
better abort and return an error (or report “not in recovery” when
using NO_THROW). At that point, the meaning of the LSN being waited
for may have changed due to the timeline switch and the transition
from standby to primary. An LSN such as 0/5000000 on TLI 2 can
represent entirely different WAL content from 0/5000000 on TLI 1.
Allowing the wait to silently continue across promotion risks giving
users a false sense of safety—for example, interpreting “wait
completed” as “the original data is now durable,” which would no
longer be true.

--
Best,
Xuneng

  69. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-12-25T11:13:36Z

    Hi, Xuneng!

On Mon, Dec 22, 2025 at 9:57 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> On Sun, Dec 21, 2025 at 12:37 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi Alexander,
> >
> > Thanks for your feedback!
> >
> > > I see that we can't specify WAIT_LSN_TYPE_PRIMARY_FLUSH by setting
> > > mode parameter.  Should we allow this?
> >
> > I think this constraint could be relaxed if needed. I was previously
> > unsure about the use cases.
>
> Flush mode on the primary seems useful when synchronous_commit is set
> to off [1]. In that mode, a transaction in primary may return success
> before its WAL is durably flushed to disk, trading durability for
> lower latency. A “wait for primary flush” operation provides an
> explicit durability barrier for cases where applications or tools
> occasionally need stronger guarantees.
>
> [1] https://postgresqlco.nf/doc/en/param/synchronous_commit/
>
> > > If we allow specifying WAIT_LSN_TYPE_PRIMARY_FLUSH, should it be
> > > separate mode value or the same with WAIT_LSN_TYPE_STANDBY_FLUSH?  In
> > > principle, we could encode both as just 'flush' mode, and detect which
> > > WaitLSNType to pick by checking if recovery is in progress.  However,
> > > how should we then react to unreached flush location after standby
> > > promotion (technically it could be still reached but on the different
> > > timeline)?
> > >
> >
> > Technically, we can use 'flush' mode to specify WAIT FOR behavior in
> > both primary and replica. Currently, wait for commands error out if
> > promotion occurs since: either the requested LSN type does not exist
> > on the primary, or we do not yet have the infrastructure to support
> > continuing the wait. If we allow waiting for flush on the primary as a
> > user-visible command and the wake-up calls for flush in primary are
> > introduced, the question becomes whether we should still abort the
> > wait on promotion, or continue waiting—as you noted—given that the
> > target LSN might still be reached, albeit on a different timeline. The
> > question behind this might be: do users care and should be aware of
> > the state change of the server while waiting? If they do, then we
> > better stop the waiting and report the error. In this case, I am
> > inclined to to break the unified flush mode to something like
> > primary_flush/standby_flush mode and
> > WAIT_LSN_TYPE_PRIMARY_FLUSH/WAIT_LSN_TYPE_STANDBY_FLUSH respectively.
> >
>
> After further consideration, it also seems reasonable to use a single,
> unified flush mode that works on both primary and standby servers,
> provided its semantics are clearly documented to avoid the potential
> confusion on failure. I don’t have a strong preference between these
> two and would be interested in your thoughts.
>
> If a standby is promoted while a session is waiting, the command
> better abort and return an error (or report “not in recovery” when
> using NO_THROW). At that point, the meaning of the LSN being waited
> for may have changed due to the timeline switch and the transition
> from standby to primary. An LSN such as 0/5000000 on TLI 2 can
> represent entirely different WAL content from 0/5000000 on TLI 1.
> Allowing the wait to silently continue across promotion risks giving
> users a false sense of safety—for example, interpreting “wait
> completed” as “the original data is now durable,” which would no
> longer be true.

Agree, but there is still risk that promotion happens after user send
the query but before we started to wait.  In this case we will still
silently start to wait on primary, while user probably meant to wait
on replica.  Probably it would be safer to have separate user-visible
modes for waiting on primary and on replica?

------
Regards,
Alexander Korotkov
Supabase

  70. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-12-25T12:52:34Z

    Hi Alexander,

On Thu, Dec 25, 2025 at 7:13 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> Hi, Xuneng!
>
> On Mon, Dec 22, 2025 at 9:57 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > On Sun, Dec 21, 2025 at 12:37 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > Thanks for your feedback!
> > >
> > > > I see that we can't specify WAIT_LSN_TYPE_PRIMARY_FLUSH by setting
> > > > mode parameter.  Should we allow this?
> > >
> > > I think this constraint could be relaxed if needed. I was previously
> > > unsure about the use cases.
> >
> > Flush mode on the primary seems useful when synchronous_commit is set
> > to off [1]. In that mode, a transaction in primary may return success
> > before its WAL is durably flushed to disk, trading durability for
> > lower latency. A “wait for primary flush” operation provides an
> > explicit durability barrier for cases where applications or tools
> > occasionally need stronger guarantees.
> >
> > [1] https://postgresqlco.nf/doc/en/param/synchronous_commit/
> >
> > > > If we allow specifying WAIT_LSN_TYPE_PRIMARY_FLUSH, should it be
> > > > separate mode value or the same with WAIT_LSN_TYPE_STANDBY_FLUSH?  In
> > > > principle, we could encode both as just 'flush' mode, and detect which
> > > > WaitLSNType to pick by checking if recovery is in progress.  However,
> > > > how should we then react to unreached flush location after standby
> > > > promotion (technically it could be still reached but on the different
> > > > timeline)?
> > > >
> > >
> > > Technically, we can use 'flush' mode to specify WAIT FOR behavior in
> > > both primary and replica. Currently, wait for commands error out if
> > > promotion occurs since: either the requested LSN type does not exist
> > > on the primary, or we do not yet have the infrastructure to support
> > > continuing the wait. If we allow waiting for flush on the primary as a
> > > user-visible command and the wake-up calls for flush in primary are
> > > introduced, the question becomes whether we should still abort the
> > > wait on promotion, or continue waiting—as you noted—given that the
> > > target LSN might still be reached, albeit on a different timeline. The
> > > question behind this might be: do users care and should be aware of
> > > the state change of the server while waiting? If they do, then we
> > > better stop the waiting and report the error. In this case, I am
> > > inclined to to break the unified flush mode to something like
> > > primary_flush/standby_flush mode and
> > > WAIT_LSN_TYPE_PRIMARY_FLUSH/WAIT_LSN_TYPE_STANDBY_FLUSH respectively.
> > >
> >
> > After further consideration, it also seems reasonable to use a single,
> > unified flush mode that works on both primary and standby servers,
> > provided its semantics are clearly documented to avoid the potential
> > confusion on failure. I don’t have a strong preference between these
> > two and would be interested in your thoughts.
> >
> > If a standby is promoted while a session is waiting, the command
> > better abort and return an error (or report “not in recovery” when
> > using NO_THROW). At that point, the meaning of the LSN being waited
> > for may have changed due to the timeline switch and the transition
> > from standby to primary. An LSN such as 0/5000000 on TLI 2 can
> > represent entirely different WAL content from 0/5000000 on TLI 1.
> > Allowing the wait to silently continue across promotion risks giving
> > users a false sense of safety—for example, interpreting “wait
> > completed” as “the original data is now durable,” which would no
> > longer be true.
>
> Agree, but there is still risk that promotion happens after user send
> the query but before we started to wait.  In this case we will still
> silently start to wait on primary, while user probably meant to wait
> on replica.  Probably it would be safer to have separate user-visible
> modes for waiting on primary and on replica?
>

Thanks for your thoughts. You're right about the race condition. If
promotion happens between query submission and execution, a unified
'flush' mode could silently switch semantics without the user knowing.
Separate modes like 'standby_flush' and 'primary_flush' would make
user intent explicit and catch this case with an error, which is
safer. Do these two terms look reasonable to you, or would you suggest
better names? If they look ok, I plan to update the implementation to
use these two modes.


--
Best,
Xuneng

  71. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-12-25T16:34:02Z

    On Thu, Dec 25, 2025 at 2:52 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> On Thu, Dec 25, 2025 at 7:13 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >
> > Hi, Xuneng!
> >
> > On Mon, Dec 22, 2025 at 9:57 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > >
> > > On Sun, Dec 21, 2025 at 12:37 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > >
> > > > Hi Alexander,
> > > >
> > > > Thanks for your feedback!
> > > >
> > > > > I see that we can't specify WAIT_LSN_TYPE_PRIMARY_FLUSH by setting
> > > > > mode parameter.  Should we allow this?
> > > >
> > > > I think this constraint could be relaxed if needed. I was previously
> > > > unsure about the use cases.
> > >
> > > Flush mode on the primary seems useful when synchronous_commit is set
> > > to off [1]. In that mode, a transaction in primary may return success
> > > before its WAL is durably flushed to disk, trading durability for
> > > lower latency. A “wait for primary flush” operation provides an
> > > explicit durability barrier for cases where applications or tools
> > > occasionally need stronger guarantees.
> > >
> > > [1] https://postgresqlco.nf/doc/en/param/synchronous_commit/
> > >
> > > > > If we allow specifying WAIT_LSN_TYPE_PRIMARY_FLUSH, should it be
> > > > > separate mode value or the same with WAIT_LSN_TYPE_STANDBY_FLUSH?  In
> > > > > principle, we could encode both as just 'flush' mode, and detect which
> > > > > WaitLSNType to pick by checking if recovery is in progress.  However,
> > > > > how should we then react to unreached flush location after standby
> > > > > promotion (technically it could be still reached but on the different
> > > > > timeline)?
> > > > >
> > > >
> > > > Technically, we can use 'flush' mode to specify WAIT FOR behavior in
> > > > both primary and replica. Currently, wait for commands error out if
> > > > promotion occurs since: either the requested LSN type does not exist
> > > > on the primary, or we do not yet have the infrastructure to support
> > > > continuing the wait. If we allow waiting for flush on the primary as a
> > > > user-visible command and the wake-up calls for flush in primary are
> > > > introduced, the question becomes whether we should still abort the
> > > > wait on promotion, or continue waiting—as you noted—given that the
> > > > target LSN might still be reached, albeit on a different timeline. The
> > > > question behind this might be: do users care and should be aware of
> > > > the state change of the server while waiting? If they do, then we
> > > > better stop the waiting and report the error. In this case, I am
> > > > inclined to to break the unified flush mode to something like
> > > > primary_flush/standby_flush mode and
> > > > WAIT_LSN_TYPE_PRIMARY_FLUSH/WAIT_LSN_TYPE_STANDBY_FLUSH respectively.
> > > >
> > >
> > > After further consideration, it also seems reasonable to use a single,
> > > unified flush mode that works on both primary and standby servers,
> > > provided its semantics are clearly documented to avoid the potential
> > > confusion on failure. I don’t have a strong preference between these
> > > two and would be interested in your thoughts.
> > >
> > > If a standby is promoted while a session is waiting, the command
> > > better abort and return an error (or report “not in recovery” when
> > > using NO_THROW). At that point, the meaning of the LSN being waited
> > > for may have changed due to the timeline switch and the transition
> > > from standby to primary. An LSN such as 0/5000000 on TLI 2 can
> > > represent entirely different WAL content from 0/5000000 on TLI 1.
> > > Allowing the wait to silently continue across promotion risks giving
> > > users a false sense of safety—for example, interpreting “wait
> > > completed” as “the original data is now durable,” which would no
> > > longer be true.
> >
> > Agree, but there is still risk that promotion happens after user send
> > the query but before we started to wait.  In this case we will still
> > silently start to wait on primary, while user probably meant to wait
> > on replica.  Probably it would be safer to have separate user-visible
> > modes for waiting on primary and on replica?
> >
>
> Thanks for your thoughts. You're right about the race condition. If
> promotion happens between query submission and execution, a unified
> 'flush' mode could silently switch semantics without the user knowing.
> Separate modes like 'standby_flush' and 'primary_flush' would make
> user intent explicit and catch this case with an error, which is
> safer. Do these two terms look reasonable to you, or would you suggest
> better names? If they look ok, I plan to update the implementation to
> use these two modes.

Thank you, Xuneng.  'standby_flush' and 'primary_flush' look good for
me.  Please, go ahead.  I think we should name other modes
'standby_write' and 'standby_replay' for the sake of unity.

------
Regards,
Alexander Korotkov
Supabase

  72. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-12-26T00:31:26Z

    Hi,

On Fri, Dec 26, 2025 at 12:34 AM Alexander Korotkov
<aekorotkov@gmail.com> wrote:
>
> On Thu, Dec 25, 2025 at 2:52 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > On Thu, Dec 25, 2025 at 7:13 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > >
> > > Hi, Xuneng!
> > >
> > > On Mon, Dec 22, 2025 at 9:57 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > >
> > > > On Sun, Dec 21, 2025 at 12:37 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > > >
> > > > > Hi Alexander,
> > > > >
> > > > > Thanks for your feedback!
> > > > >
> > > > > > I see that we can't specify WAIT_LSN_TYPE_PRIMARY_FLUSH by setting
> > > > > > mode parameter.  Should we allow this?
> > > > >
> > > > > I think this constraint could be relaxed if needed. I was previously
> > > > > unsure about the use cases.
> > > >
> > > > Flush mode on the primary seems useful when synchronous_commit is set
> > > > to off [1]. In that mode, a transaction in primary may return success
> > > > before its WAL is durably flushed to disk, trading durability for
> > > > lower latency. A “wait for primary flush” operation provides an
> > > > explicit durability barrier for cases where applications or tools
> > > > occasionally need stronger guarantees.
> > > >
> > > > [1] https://postgresqlco.nf/doc/en/param/synchronous_commit/
> > > >
> > > > > > If we allow specifying WAIT_LSN_TYPE_PRIMARY_FLUSH, should it be
> > > > > > separate mode value or the same with WAIT_LSN_TYPE_STANDBY_FLUSH?  In
> > > > > > principle, we could encode both as just 'flush' mode, and detect which
> > > > > > WaitLSNType to pick by checking if recovery is in progress.  However,
> > > > > > how should we then react to unreached flush location after standby
> > > > > > promotion (technically it could be still reached but on the different
> > > > > > timeline)?
> > > > > >
> > > > >
> > > > > Technically, we can use 'flush' mode to specify WAIT FOR behavior in
> > > > > both primary and replica. Currently, wait for commands error out if
> > > > > promotion occurs since: either the requested LSN type does not exist
> > > > > on the primary, or we do not yet have the infrastructure to support
> > > > > continuing the wait. If we allow waiting for flush on the primary as a
> > > > > user-visible command and the wake-up calls for flush in primary are
> > > > > introduced, the question becomes whether we should still abort the
> > > > > wait on promotion, or continue waiting—as you noted—given that the
> > > > > target LSN might still be reached, albeit on a different timeline. The
> > > > > question behind this might be: do users care and should be aware of
> > > > > the state change of the server while waiting? If they do, then we
> > > > > better stop the waiting and report the error. In this case, I am
> > > > > inclined to to break the unified flush mode to something like
> > > > > primary_flush/standby_flush mode and
> > > > > WAIT_LSN_TYPE_PRIMARY_FLUSH/WAIT_LSN_TYPE_STANDBY_FLUSH respectively.
> > > > >
> > > >
> > > > After further consideration, it also seems reasonable to use a single,
> > > > unified flush mode that works on both primary and standby servers,
> > > > provided its semantics are clearly documented to avoid the potential
> > > > confusion on failure. I don’t have a strong preference between these
> > > > two and would be interested in your thoughts.
> > > >
> > > > If a standby is promoted while a session is waiting, the command
> > > > better abort and return an error (or report “not in recovery” when
> > > > using NO_THROW). At that point, the meaning of the LSN being waited
> > > > for may have changed due to the timeline switch and the transition
> > > > from standby to primary. An LSN such as 0/5000000 on TLI 2 can
> > > > represent entirely different WAL content from 0/5000000 on TLI 1.
> > > > Allowing the wait to silently continue across promotion risks giving
> > > > users a false sense of safety—for example, interpreting “wait
> > > > completed” as “the original data is now durable,” which would no
> > > > longer be true.
> > >
> > > Agree, but there is still risk that promotion happens after user send
> > > the query but before we started to wait.  In this case we will still
> > > silently start to wait on primary, while user probably meant to wait
> > > on replica.  Probably it would be safer to have separate user-visible
> > > modes for waiting on primary and on replica?
> > >
> >
> > Thanks for your thoughts. You're right about the race condition. If
> > promotion happens between query submission and execution, a unified
> > 'flush' mode could silently switch semantics without the user knowing.
> > Separate modes like 'standby_flush' and 'primary_flush' would make
> > user intent explicit and catch this case with an error, which is
> > safer. Do these two terms look reasonable to you, or would you suggest
> > better names? If they look ok, I plan to update the implementation to
> > use these two modes.
>
> Thank you, Xuneng.  'standby_flush' and 'primary_flush' look good for
> me.  Please, go ahead.  I think we should name other modes
> 'standby_write' and 'standby_replay' for the sake of unity.
>

Thanks. Yeah, renaming existing modes to  'standby_write' and
'standby_replay' also makes sense to me.

-- 
Best,
Xuneng

  73. Re: Implement waiting for wal lsn replay: reloaded

    Chao Li <li.evan.chao@gmail.com> — 2025-12-26T08:24:39Z

    
> On Dec 19, 2025, at 10:49, Xuneng Zhou <xunengzhou@gmail.com> wrote:
> 
> Hi,
> 
> On Thu, Dec 18, 2025 at 8:25 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>> 
>> On Thu, Dec 18, 2025 at 2:24 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>>> On Thu, Dec 18, 2025 at 6:38 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>>>> 
>>>> Hi, Xuneng!
>>>> 
>>>> On Tue, Dec 16, 2025 at 6:46 AM Xuneng Zhou <xunengzhou@gmail.com> 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/

  74. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-12-26T16:15:05Z

    Hi Chao,

Thanks a lot for your review!

On Fri, Dec 26, 2025 at 4:25 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On Dec 19, 2025, at 10:49, Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi,
> >
> > On Thu, Dec 18, 2025 at 8:25 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >>
> >> On Thu, Dec 18, 2025 at 2:24 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >>> On Thu, Dec 18, 2025 at 6:38 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >>>>
> >>>> Hi, Xuneng!
> >>>>
> >>>> On Tue, Dec 16, 2025 at 6:46 AM Xuneng Zhou <xunengzhou@gmail.com> 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).

My take is that Assert(false) alone might not be enough here, since
assertions vanish in non-assert builds. An unexpected lsnType is a
real bug even in production, so keeping a hard error plus
pg_unreachable() seems to be a safer pattern. It also acts as a
guardrail for future extensions — if new wait types are added without
updating this code, we’ll fail loudly rather than silently returning
an incorrect LSN. Assert(i >= 0 && i < WAIT_LSN_TYPE_COUNT) was added
to the top of the function.

> 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?

Yeah, putting MODE into the error message is cleaner. It's done in v8.

> 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.

+ 1.

> 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”.
>

It makes sense to me. They are incorporated into v8.

>
> 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.

 The sentence is retained.

> 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?
>

This seems unnecessary. I also realized that my earlier approach in
patch 4 may have been semantically incorrect — it could end up waiting
for the LSN to replay/write/flush on the node itself, rather than on
the downstream standby, which defeats the purpose of
wait_for_catchup(). Patch 4 attempts to address this by running WAIT
FOR LSN on the standby itself.

Support for primary-flush waiting and the refactoring of existing
modes have been also incorporated in v8 following Alexander’s
feedback. The major updates are in patches 2 and 4. Please check.

--
Best,
Xuneng

  75. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-12-30T02:12:28Z

    Hi,

On Sat, Dec 27, 2025 at 12:15 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi Chao,
>
> Thanks a lot for your review!
>
> On Fri, Dec 26, 2025 at 4:25 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >
> >
> >
> > > On Dec 19, 2025, at 10:49, Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > On Thu, Dec 18, 2025 at 8:25 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > >>
> > >> On Thu, Dec 18, 2025 at 2:24 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > >>> On Thu, Dec 18, 2025 at 6:38 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > >>>>
> > >>>> Hi, Xuneng!
> > >>>>
> > >>>> On Tue, Dec 16, 2025 at 6:46 AM Xuneng Zhou <xunengzhou@gmail.com> 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).
>
> My take is that Assert(false) alone might not be enough here, since
> assertions vanish in non-assert builds. An unexpected lsnType is a
> real bug even in production, so keeping a hard error plus
> pg_unreachable() seems to be a safer pattern. It also acts as a
> guardrail for future extensions — if new wait types are added without
> updating this code, we’ll fail loudly rather than silently returning
> an incorrect LSN. Assert(i >= 0 && i < WAIT_LSN_TYPE_COUNT) was added
> to the top of the function.
>
> > 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?
>
> Yeah, putting MODE into the error message is cleaner. It's done in v8.
>
> > 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.
>
> + 1.
>
> > 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”.
> >
>
> It makes sense to me. They are incorporated into v8.
>
> >
> > 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.
>
>  The sentence is retained.
>
> > 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?
> >
>
> This seems unnecessary. I also realized that my earlier approach in
> patch 4 may have been semantically incorrect — it could end up waiting
> for the LSN to replay/write/flush on the node itself, rather than on
> the downstream standby, which defeats the purpose of
> wait_for_catchup(). Patch 4 attempts to address this by running WAIT
> FOR LSN on the standby itself.
>
> Support for primary-flush waiting and the refactoring of existing
> modes have been also incorporated in v8 following Alexander’s
> feedback. The major updates are in patches 2 and 4. Please check.
>

Added WaitLSNTypeDesc to typedefs.list in v9 patch 2.

-- 
Best,
Xuneng

  76. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-12-30T02:42:09Z

    Hi,

On Tue, Dec 30, 2025 at 10:12 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> On Sat, Dec 27, 2025 at 12:15 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi Chao,
> >
> > Thanks a lot for your review!
> >
> > On Fri, Dec 26, 2025 at 4:25 PM Chao Li <li.evan.chao@gmail.com> wrote:
> > >
> > >
> > >
> > > > On Dec 19, 2025, at 10:49, Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Thu, Dec 18, 2025 at 8:25 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > > >>
> > > >> On Thu, Dec 18, 2025 at 2:24 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > >>> On Thu, Dec 18, 2025 at 6:38 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > > >>>>
> > > >>>> Hi, Xuneng!
> > > >>>>
> > > >>>> On Tue, Dec 16, 2025 at 6:46 AM Xuneng Zhou <xunengzhou@gmail.com> 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).
> >
> > My take is that Assert(false) alone might not be enough here, since
> > assertions vanish in non-assert builds. An unexpected lsnType is a
> > real bug even in production, so keeping a hard error plus
> > pg_unreachable() seems to be a safer pattern. It also acts as a
> > guardrail for future extensions — if new wait types are added without
> > updating this code, we’ll fail loudly rather than silently returning
> > an incorrect LSN. Assert(i >= 0 && i < WAIT_LSN_TYPE_COUNT) was added
> > to the top of the function.
> >
> > > 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?
> >
> > Yeah, putting MODE into the error message is cleaner. It's done in v8.
> >
> > > 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.
> >
> > + 1.
> >
> > > 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”.
> > >
> >
> > It makes sense to me. They are incorporated into v8.
> >
> > >
> > > 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.
> >
> >  The sentence is retained.
> >
> > > 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?
> > >
> >
> > This seems unnecessary. I also realized that my earlier approach in
> > patch 4 may have been semantically incorrect — it could end up waiting
> > for the LSN to replay/write/flush on the node itself, rather than on
> > the downstream standby, which defeats the purpose of
> > wait_for_catchup(). Patch 4 attempts to address this by running WAIT
> > FOR LSN on the standby itself.
> >
> > Support for primary-flush waiting and the refactoring of existing
> > modes have been also incorporated in v8 following Alexander’s
> > feedback. The major updates are in patches 2 and 4. Please check.
> >
>
> Added WaitLSNTypeDesc to typedefs.list in v9 patch 2.
>

Run pgindent using the updated typedefs.list.

-- 
Best,
Xuneng

  77. Re: Implement waiting for wal lsn replay: reloaded

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-12-30T03:14:42Z

    On 2025-Dec-27, Xuneng Zhou wrote:

> On Fri, Dec 26, 2025 at 4:25 PM Chao Li <li.evan.chao@gmail.com> wrote:

> > 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?
> 
> Yeah, putting MODE into the error message is cleaner. It's done in v8.

The reason not to do that (and also put WAIT in a separate string) is so
that the message is identicla to other messages and thus requires no
separate translation, specifically
  errmsg("unrecognized value for %s option \"%s\": \"%s\"", ...)

See commit 502e256f2262.  Please use that form.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El sabio habla porque tiene algo que decir;
el tonto, porque tiene que decir algo" (Platon).

  78. Re: Implement waiting for wal lsn replay: reloaded

    Chao Li <li.evan.chao@gmail.com> — 2025-12-30T03:24:37Z

    
> On Dec 30, 2025, at 11:14, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> 
> On 2025-Dec-27, Xuneng Zhou wrote:
> 
>> On Fri, Dec 26, 2025 at 4:25 PM Chao Li <li.evan.chao@gmail.com> wrote:
> 
>>> 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?
>> 
>> Yeah, putting MODE into the error message is cleaner. It's done in v8.
> 
> The reason not to do that (and also put WAIT in a separate string) is so
> that the message is identicla to other messages and thus requires no
> separate translation, specifically
>  errmsg("unrecognized value for %s option \"%s\": \"%s\"", ...)
> 
> See commit 502e256f2262.  Please use that form.
> 

To follow 502e256f2262, it should use “%s” for “WAIT” as well. I raised the comment because I saw “WAIT” is the format strings, thus “MODE” can be there as well.

So, we should do a similar change like:
```
-                                                errmsg("unrecognized value for EXPLAIN option \"%s\": \"%s\"",
-                                                               opt->defname, p),
+                                                errmsg("unrecognized value for %s option \"%s\": \"%s\"",
+                                                               "EXPLAIN", opt->defname, p),
```

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

  79. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-12-30T06:19:27Z

    Hi,

On Tue, Dec 30, 2025 at 11:25 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On Dec 30, 2025, at 11:14, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> >
> > On 2025-Dec-27, Xuneng Zhou wrote:
> >
> >> On Fri, Dec 26, 2025 at 4:25 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >
> >>> 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?
> >>
> >> Yeah, putting MODE into the error message is cleaner. It's done in v8.
> >
> > The reason not to do that (and also put WAIT in a separate string) is so
> > that the message is identicla to other messages and thus requires no
> > separate translation, specifically
> >  errmsg("unrecognized value for %s option \"%s\": \"%s\"", ...)
> >
> > See commit 502e256f2262.  Please use that form.
> >
>
> To follow 502e256f2262, it should use “%s” for “WAIT” as well. I raised the comment because I saw “WAIT” is the format strings, thus “MODE” can be there as well.
>
> So, we should do a similar change like:
> ```
> -                                                errmsg("unrecognized value for EXPLAIN option \"%s\": \"%s\"",
> -                                                               opt->defname, p),
> +                                                errmsg("unrecognized value for %s option \"%s\": \"%s\"",
> +                                                               "EXPLAIN", opt->defname, p),
> ```
>

Thanks for raising this and clarifying the rationale. I've made the
modification per your input.

-- 
Best,
Xuneng

  80. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Lakhin <exclusion@gmail.com> — 2026-05-19T20:00:00Z

    Hello Alexander and Xuneng,

06.04.2026 22:49, Alexander Korotkov wrote:
> Thank you, I've pushed your version of patchset.  I made two minor
> corrections for patch #2: mention default mode value in the header
> comment, and fallback to polling on has_wal_read_bug sparc64+ext4 bug.

I discovered a new test failure, that is apparently caused by new
wait_for_catchup() implementation [1]:
[06:20:23.110](1.069s) not ok 8 - check that the slot state changes to "extended"
[06:20:23.110](0.001s) #   Failed test 'check that the slot state changes to "extended"'
#   at /Users/ec2-user/bf/goldfish/HEAD/pgsql/src/test/recovery/t/019_replslot_limit.pl line 140.
[06:20:23.111](0.000s) #          got: 'unreserved'
#     expected: 'extended'
[06:20:23.231](0.120s) not ok 9 - check that the slot state changes to "unreserved"
[06:20:23.231](0.000s) #   Failed test 'check that the slot state changes to "unreserved"'
#   at /Users/ec2-user/bf/goldfish/HEAD/pgsql/src/test/recovery/t/019_replslot_limit.pl line 152.
[06:20:23.231](0.000s) #          got: 'lost|'
#     expected: 'unreserved|t'

I've managed to reproduce such failures with:
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index 07eac07b9ce..493ce92674e 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -1143,2 +1143,3 @@ XLogWalRcvSendReply(bool force, bool requestReply, bool checkApply)

+pg_usleep(10000);
      /* Get current timestamp. */
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 04aa770d981..19cda3a6b51 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -2521,2 +2521,3 @@ ProcessStandbyReplyMessage(void)

+pg_usleep(100000);
      /* the caller already consumed the msgtype byte */

Concretely, a loop:
for i in {1..100}; do echo "ITERATION $i"; PROVE_TESTS="t/019*" make -s check -C src/test/recovery/ || break; done
failed for me on iterations 2, 1, 7:
ITERATION 7
# +++ tap check in src/test/recovery +++
t/019_replslot_limit.pl .. 8/?
#   Failed test 'check that the slot state changes to "extended"'
#   at t/019_replslot_limit.pl line 140.
#          got: 'unreserved'
#     expected: 'extended'
t/019_replslot_limit.pl .. 26/? # Looks like you failed 1 test of 26.
t/019_replslot_limit.pl .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/26 subtests

With "WAIT FOR LSN" in wait_for_catchup() disabled, 100 iterations
passed.

Having extra logging added, I could see the key difference.
Failed run:
2026-05-19 22:01:37.968 EEST client backend[3632148] 019_replslot_limit.pl LOG:  !!!GetWALAvailability| targetLSN: 
0/016C0000, targetSeg: 22, oldestSlotSeg: 23, oldestSegMaxWalSize: 24, oldestSeg: 22
2026-05-19 22:01:37.968 EEST client backend[3632148] 019_replslot_limit.pl STATEMENT:  SELECT wal_status FROM 
pg_replication_slots WHERE slot_name = 'rep1'
vs
Successful run:
2026-05-19 22:04:18.102 EEST client backend[3633761] 019_replslot_limit.pl LOG:  !!!GetWALAvailability| targetLSN: 
0/01700000, targetSeg: 23, oldestSlotSeg: 23, oldestSegMaxWalSize: 24, oldestSeg: 23
2026-05-19 22:04:18.102 EEST client backend[3633761] 019_replslot_limit.pl STATEMENT:  SELECT wal_status FROM 
pg_replication_slots WHERE slot_name = 'rep1'

That is, with WAIT FOR LSN, primary in this test may advance
slot->data.restart_lsn to the expected position after wait_for_catchup()
returns.

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=goldfish&dt=2026-05-13%2006%3A15%3A03

Best regards,
Alexander

  81. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2026-05-20T03:30:10Z

    Hi,

On Tue, May 19, 2026 at 1:00 PM Alexander Lakhin <exclusion@gmail.com> wrote:
>
> Hello Alexander and Xuneng,
>
> 06.04.2026 22:49, Alexander Korotkov wrote:
>
> Thank you, I've pushed your version of patchset.  I made two minor
> corrections for patch #2: mention default mode value in the header
> comment, and fallback to polling on has_wal_read_bug sparc64+ext4 bug.
>
>
> I discovered a new test failure, that is apparently caused by new
> wait_for_catchup() implementation [1]:
> [06:20:23.110](1.069s) not ok 8 - check that the slot state changes to "extended"
> [06:20:23.110](0.001s) #   Failed test 'check that the slot state changes to "extended"'
> #   at /Users/ec2-user/bf/goldfish/HEAD/pgsql/src/test/recovery/t/019_replslot_limit.pl line 140.
> [06:20:23.111](0.000s) #          got: 'unreserved'
> #     expected: 'extended'
> [06:20:23.231](0.120s) not ok 9 - check that the slot state changes to "unreserved"
> [06:20:23.231](0.000s) #   Failed test 'check that the slot state changes to "unreserved"'
> #   at /Users/ec2-user/bf/goldfish/HEAD/pgsql/src/test/recovery/t/019_replslot_limit.pl line 152.
> [06:20:23.231](0.000s) #          got: 'lost|'
> #     expected: 'unreserved|t'
>
> I've managed to reproduce such failures with:
> diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
> index 07eac07b9ce..493ce92674e 100644
> --- a/src/backend/replication/walreceiver.c
> +++ b/src/backend/replication/walreceiver.c
> @@ -1143,2 +1143,3 @@ XLogWalRcvSendReply(bool force, bool requestReply, bool checkApply)
>
> +pg_usleep(10000);
>      /* Get current timestamp. */
> diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
> index 04aa770d981..19cda3a6b51 100644
> --- a/src/backend/replication/walsender.c
> +++ b/src/backend/replication/walsender.c
> @@ -2521,2 +2521,3 @@ ProcessStandbyReplyMessage(void)
>
> +pg_usleep(100000);
>      /* the caller already consumed the msgtype byte */
>
> Concretely, a loop:
> for i in {1..100}; do echo "ITERATION $i"; PROVE_TESTS="t/019*" make -s check -C src/test/recovery/ || break; done
> failed for me on iterations 2, 1, 7:
> ITERATION 7
> # +++ tap check in src/test/recovery +++
> t/019_replslot_limit.pl .. 8/?
> #   Failed test 'check that the slot state changes to "extended"'
> #   at t/019_replslot_limit.pl line 140.
> #          got: 'unreserved'
> #     expected: 'extended'
> t/019_replslot_limit.pl .. 26/? # Looks like you failed 1 test of 26.
> t/019_replslot_limit.pl .. Dubious, test returned 1 (wstat 256, 0x100)
> Failed 1/26 subtests
>
> With "WAIT FOR LSN" in wait_for_catchup() disabled, 100 iterations
> passed.
>
> Having extra logging added, I could see the key difference.
> Failed run:
> 2026-05-19 22:01:37.968 EEST client backend[3632148] 019_replslot_limit.pl LOG:  !!!GetWALAvailability| targetLSN: 0/016C0000, targetSeg: 22, oldestSlotSeg: 23, oldestSegMaxWalSize: 24, oldestSeg: 22
> 2026-05-19 22:01:37.968 EEST client backend[3632148] 019_replslot_limit.pl STATEMENT:  SELECT wal_status FROM pg_replication_slots WHERE slot_name = 'rep1'
> vs
> Successful run:
> 2026-05-19 22:04:18.102 EEST client backend[3633761] 019_replslot_limit.pl LOG:  !!!GetWALAvailability| targetLSN: 0/01700000, targetSeg: 23, oldestSlotSeg: 23, oldestSegMaxWalSize: 24, oldestSeg: 23
> 2026-05-19 22:04:18.102 EEST client backend[3633761] 019_replslot_limit.pl STATEMENT:  SELECT wal_status FROM pg_replication_slots WHERE slot_name = 'rep1'
>
> That is, with WAIT FOR LSN, primary in this test may advance
> slot->data.restart_lsn to the expected position after wait_for_catchup()
> returns.
>
> [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=goldfish&dt=2026-05-13%2006%3A15%3A03

Thanks for reporting this issue.

I think this is related to the semantic change made earlier:
wait_for_catchup() now returns once the standby itself has reached the
target LSN, rather than waiting until the primary observes that
position via pg_stat_replication.

As a result, the primary may not yet have processed the standby
feedback needed to advance the slot's restart_lsn when
wait_for_catchup() returns.

Actually, I was aware of this semantic change and previously thought
it might be harmless. But this failure appears to disprove that. I'll
prepare a patch to fix this shortly.

-- 
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

  82. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2026-05-20T05:18:07Z

    On Tue, May 19, 2026 at 8:30 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> On Tue, May 19, 2026 at 1:00 PM Alexander Lakhin <exclusion@gmail.com> wrote:
> >
> > Hello Alexander and Xuneng,
> >
> > 06.04.2026 22:49, Alexander Korotkov wrote:
> >
> > Thank you, I've pushed your version of patchset.  I made two minor
> > corrections for patch #2: mention default mode value in the header
> > comment, and fallback to polling on has_wal_read_bug sparc64+ext4 bug.
> >
> >
> > I discovered a new test failure, that is apparently caused by new
> > wait_for_catchup() implementation [1]:
> > [06:20:23.110](1.069s) not ok 8 - check that the slot state changes to "extended"
> > [06:20:23.110](0.001s) #   Failed test 'check that the slot state changes to "extended"'
> > #   at /Users/ec2-user/bf/goldfish/HEAD/pgsql/src/test/recovery/t/019_replslot_limit.pl line 140.
> > [06:20:23.111](0.000s) #          got: 'unreserved'
> > #     expected: 'extended'
> > [06:20:23.231](0.120s) not ok 9 - check that the slot state changes to "unreserved"
> > [06:20:23.231](0.000s) #   Failed test 'check that the slot state changes to "unreserved"'
> > #   at /Users/ec2-user/bf/goldfish/HEAD/pgsql/src/test/recovery/t/019_replslot_limit.pl line 152.
> > [06:20:23.231](0.000s) #          got: 'lost|'
> > #     expected: 'unreserved|t'
> >
> > I've managed to reproduce such failures with:
> > diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
> > index 07eac07b9ce..493ce92674e 100644
> > --- a/src/backend/replication/walreceiver.c
> > +++ b/src/backend/replication/walreceiver.c
> > @@ -1143,2 +1143,3 @@ XLogWalRcvSendReply(bool force, bool requestReply, bool checkApply)
> >
> > +pg_usleep(10000);
> >      /* Get current timestamp. */
> > diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
> > index 04aa770d981..19cda3a6b51 100644
> > --- a/src/backend/replication/walsender.c
> > +++ b/src/backend/replication/walsender.c
> > @@ -2521,2 +2521,3 @@ ProcessStandbyReplyMessage(void)
> >
> > +pg_usleep(100000);
> >      /* the caller already consumed the msgtype byte */
> >
> > Concretely, a loop:
> > for i in {1..100}; do echo "ITERATION $i"; PROVE_TESTS="t/019*" make -s check -C src/test/recovery/ || break; done
> > failed for me on iterations 2, 1, 7:
> > ITERATION 7
> > # +++ tap check in src/test/recovery +++
> > t/019_replslot_limit.pl .. 8/?
> > #   Failed test 'check that the slot state changes to "extended"'
> > #   at t/019_replslot_limit.pl line 140.
> > #          got: 'unreserved'
> > #     expected: 'extended'
> > t/019_replslot_limit.pl .. 26/? # Looks like you failed 1 test of 26.
> > t/019_replslot_limit.pl .. Dubious, test returned 1 (wstat 256, 0x100)
> > Failed 1/26 subtests
> >
> > With "WAIT FOR LSN" in wait_for_catchup() disabled, 100 iterations
> > passed.
> >
> > Having extra logging added, I could see the key difference.
> > Failed run:
> > 2026-05-19 22:01:37.968 EEST client backend[3632148] 019_replslot_limit.pl LOG:  !!!GetWALAvailability| targetLSN: 0/016C0000, targetSeg: 22, oldestSlotSeg: 23, oldestSegMaxWalSize: 24, oldestSeg: 22
> > 2026-05-19 22:01:37.968 EEST client backend[3632148] 019_replslot_limit.pl STATEMENT:  SELECT wal_status FROM pg_replication_slots WHERE slot_name = 'rep1'
> > vs
> > Successful run:
> > 2026-05-19 22:04:18.102 EEST client backend[3633761] 019_replslot_limit.pl LOG:  !!!GetWALAvailability| targetLSN: 0/01700000, targetSeg: 23, oldestSlotSeg: 23, oldestSegMaxWalSize: 24, oldestSeg: 23
> > 2026-05-19 22:04:18.102 EEST client backend[3633761] 019_replslot_limit.pl STATEMENT:  SELECT wal_status FROM pg_replication_slots WHERE slot_name = 'rep1'
> >
> > That is, with WAIT FOR LSN, primary in this test may advance
> > slot->data.restart_lsn to the expected position after wait_for_catchup()
> > returns.
> >
> > [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=goldfish&dt=2026-05-13%2006%3A15%3A03
>
> Thanks for reporting this issue.
>
> I think this is related to the semantic change made earlier:
> wait_for_catchup() now returns once the standby itself has reached the
> target LSN, rather than waiting until the primary observes that
> position via pg_stat_replication.
>
> As a result, the primary may not yet have processed the standby
> feedback needed to advance the slot's restart_lsn when
> wait_for_catchup() returns.
>
> Actually, I was aware of this semantic change and previously thought
> it might be harmless. But this failure appears to disprove that. I'll
> prepare a patch to fix this shortly.

After some consideration, 019_replslot_limit.pl appears to the better
place to place the fix rather than by restoring the old primary-side
polling barrier in wait_for_catchup().

The new wait_for_catchup() behavior is closer to its natural
semantics: for replay/write/flush modes, it waits until the standby
itself has reached the requested LSN. The old implementation used
pg_stat_replication on the primary, which also implied that the
primary had received and processed standby feedback. That was a useful
side effect for this test, but it is not required by most callers.

019_replslot_limit.pl is different because it checks primary-side slot
state. For a physical slot, restart_lsn advances only after the
primary's walsender processes standby feedback. So the test needs an
extra condition beyond ordinary standby catchup.

The patch makes that dependency explicit: wait for the standby to
replay the target LSN, then wait for the slot's restart_lsn on the
primary to pass the same LSN. This keeps wait_for_catchup() focused on
standby catchup while making the slot-specific synchronization visible
in the test that needs it.


-- 
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

  83. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2026-05-20T12:16:45Z

    Hi, Xuneng!

On Wed, May 20, 2026 at 8:18 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> On Tue, May 19, 2026 at 8:30 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi,
> >
> > On Tue, May 19, 2026 at 1:00 PM Alexander Lakhin <exclusion@gmail.com> wrote:
> > >
> > > Hello Alexander and Xuneng,
> > >
> > > 06.04.2026 22:49, Alexander Korotkov wrote:
> > >
> > > Thank you, I've pushed your version of patchset.  I made two minor
> > > corrections for patch #2: mention default mode value in the header
> > > comment, and fallback to polling on has_wal_read_bug sparc64+ext4 bug.
> > >
> > >
> > > I discovered a new test failure, that is apparently caused by new
> > > wait_for_catchup() implementation [1]:
> > > [06:20:23.110](1.069s) not ok 8 - check that the slot state changes to "extended"
> > > [06:20:23.110](0.001s) #   Failed test 'check that the slot state changes to "extended"'
> > > #   at /Users/ec2-user/bf/goldfish/HEAD/pgsql/src/test/recovery/t/019_replslot_limit.pl line 140.
> > > [06:20:23.111](0.000s) #          got: 'unreserved'
> > > #     expected: 'extended'
> > > [06:20:23.231](0.120s) not ok 9 - check that the slot state changes to "unreserved"
> > > [06:20:23.231](0.000s) #   Failed test 'check that the slot state changes to "unreserved"'
> > > #   at /Users/ec2-user/bf/goldfish/HEAD/pgsql/src/test/recovery/t/019_replslot_limit.pl line 152.
> > > [06:20:23.231](0.000s) #          got: 'lost|'
> > > #     expected: 'unreserved|t'
> > >
> > > I've managed to reproduce such failures with:
> > > diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
> > > index 07eac07b9ce..493ce92674e 100644
> > > --- a/src/backend/replication/walreceiver.c
> > > +++ b/src/backend/replication/walreceiver.c
> > > @@ -1143,2 +1143,3 @@ XLogWalRcvSendReply(bool force, bool requestReply, bool checkApply)
> > >
> > > +pg_usleep(10000);
> > >      /* Get current timestamp. */
> > > diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
> > > index 04aa770d981..19cda3a6b51 100644
> > > --- a/src/backend/replication/walsender.c
> > > +++ b/src/backend/replication/walsender.c
> > > @@ -2521,2 +2521,3 @@ ProcessStandbyReplyMessage(void)
> > >
> > > +pg_usleep(100000);
> > >      /* the caller already consumed the msgtype byte */
> > >
> > > Concretely, a loop:
> > > for i in {1..100}; do echo "ITERATION $i"; PROVE_TESTS="t/019*" make -s check -C src/test/recovery/ || break; done
> > > failed for me on iterations 2, 1, 7:
> > > ITERATION 7
> > > # +++ tap check in src/test/recovery +++
> > > t/019_replslot_limit.pl .. 8/?
> > > #   Failed test 'check that the slot state changes to "extended"'
> > > #   at t/019_replslot_limit.pl line 140.
> > > #          got: 'unreserved'
> > > #     expected: 'extended'
> > > t/019_replslot_limit.pl .. 26/? # Looks like you failed 1 test of 26.
> > > t/019_replslot_limit.pl .. Dubious, test returned 1 (wstat 256, 0x100)
> > > Failed 1/26 subtests
> > >
> > > With "WAIT FOR LSN" in wait_for_catchup() disabled, 100 iterations
> > > passed.
> > >
> > > Having extra logging added, I could see the key difference.
> > > Failed run:
> > > 2026-05-19 22:01:37.968 EEST client backend[3632148] 019_replslot_limit.pl LOG:  !!!GetWALAvailability| targetLSN: 0/016C0000, targetSeg: 22, oldestSlotSeg: 23, oldestSegMaxWalSize: 24, oldestSeg: 22
> > > 2026-05-19 22:01:37.968 EEST client backend[3632148] 019_replslot_limit.pl STATEMENT:  SELECT wal_status FROM pg_replication_slots WHERE slot_name = 'rep1'
> > > vs
> > > Successful run:
> > > 2026-05-19 22:04:18.102 EEST client backend[3633761] 019_replslot_limit.pl LOG:  !!!GetWALAvailability| targetLSN: 0/01700000, targetSeg: 23, oldestSlotSeg: 23, oldestSegMaxWalSize: 24, oldestSeg: 23
> > > 2026-05-19 22:04:18.102 EEST client backend[3633761] 019_replslot_limit.pl STATEMENT:  SELECT wal_status FROM pg_replication_slots WHERE slot_name = 'rep1'
> > >
> > > That is, with WAIT FOR LSN, primary in this test may advance
> > > slot->data.restart_lsn to the expected position after wait_for_catchup()
> > > returns.
> > >
> > > [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=goldfish&dt=2026-05-13%2006%3A15%3A03
> >
> > Thanks for reporting this issue.
> >
> > I think this is related to the semantic change made earlier:
> > wait_for_catchup() now returns once the standby itself has reached the
> > target LSN, rather than waiting until the primary observes that
> > position via pg_stat_replication.
> >
> > As a result, the primary may not yet have processed the standby
> > feedback needed to advance the slot's restart_lsn when
> > wait_for_catchup() returns.
> >
> > Actually, I was aware of this semantic change and previously thought
> > it might be harmless. But this failure appears to disprove that. I'll
> > prepare a patch to fix this shortly.
>
> After some consideration, 019_replslot_limit.pl appears to the better
> place to place the fix rather than by restoring the old primary-side
> polling barrier in wait_for_catchup().
>
> The new wait_for_catchup() behavior is closer to its natural
> semantics: for replay/write/flush modes, it waits until the standby
> itself has reached the requested LSN. The old implementation used
> pg_stat_replication on the primary, which also implied that the
> primary had received and processed standby feedback. That was a useful
> side effect for this test, but it is not required by most callers.
>
> 019_replslot_limit.pl is different because it checks primary-side slot
> state. For a physical slot, restart_lsn advances only after the
> primary's walsender processes standby feedback. So the test needs an
> extra condition beyond ordinary standby catchup.
>
> The patch makes that dependency explicit: wait for the standby to
> replay the target LSN, then wait for the slot's restart_lsn on the
> primary to pass the same LSN. This keeps wait_for_catchup() focused on
> standby catchup while making the slot-specific synchronization visible
> in the test that needs it.

I agree with you.  But do we actually need a
wait_for_standby_and_slot_catchup() wrapper.  I think we can call
$node->wait_for_slot_catchup() directly and simplify the fix.  Check
the attached patch.

------
Regards,
Alexander Korotkov
Supabase

  84. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2026-05-23T17:09:52Z

    Hi Alexander,

On Wed, May 20, 2026 at 5:17 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> Hi, Xuneng!
>
> On Wed, May 20, 2026 at 8:18 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > On Tue, May 19, 2026 at 8:30 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, May 19, 2026 at 1:00 PM Alexander Lakhin <exclusion@gmail.com> wrote:
> > > >
> > > > Hello Alexander and Xuneng,
> > > >
> > > > 06.04.2026 22:49, Alexander Korotkov wrote:
> > > >
> > > > Thank you, I've pushed your version of patchset.  I made two minor
> > > > corrections for patch #2: mention default mode value in the header
> > > > comment, and fallback to polling on has_wal_read_bug sparc64+ext4 bug.
> > > >
> > > >
> > > > I discovered a new test failure, that is apparently caused by new
> > > > wait_for_catchup() implementation [1]:
> > > > [06:20:23.110](1.069s) not ok 8 - check that the slot state changes to "extended"
> > > > [06:20:23.110](0.001s) #   Failed test 'check that the slot state changes to "extended"'
> > > > #   at /Users/ec2-user/bf/goldfish/HEAD/pgsql/src/test/recovery/t/019_replslot_limit.pl line 140.
> > > > [06:20:23.111](0.000s) #          got: 'unreserved'
> > > > #     expected: 'extended'
> > > > [06:20:23.231](0.120s) not ok 9 - check that the slot state changes to "unreserved"
> > > > [06:20:23.231](0.000s) #   Failed test 'check that the slot state changes to "unreserved"'
> > > > #   at /Users/ec2-user/bf/goldfish/HEAD/pgsql/src/test/recovery/t/019_replslot_limit.pl line 152.
> > > > [06:20:23.231](0.000s) #          got: 'lost|'
> > > > #     expected: 'unreserved|t'
> > > >
> > > > I've managed to reproduce such failures with:
> > > > diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
> > > > index 07eac07b9ce..493ce92674e 100644
> > > > --- a/src/backend/replication/walreceiver.c
> > > > +++ b/src/backend/replication/walreceiver.c
> > > > @@ -1143,2 +1143,3 @@ XLogWalRcvSendReply(bool force, bool requestReply, bool checkApply)
> > > >
> > > > +pg_usleep(10000);
> > > >      /* Get current timestamp. */
> > > > diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
> > > > index 04aa770d981..19cda3a6b51 100644
> > > > --- a/src/backend/replication/walsender.c
> > > > +++ b/src/backend/replication/walsender.c
> > > > @@ -2521,2 +2521,3 @@ ProcessStandbyReplyMessage(void)
> > > >
> > > > +pg_usleep(100000);
> > > >      /* the caller already consumed the msgtype byte */
> > > >
> > > > Concretely, a loop:
> > > > for i in {1..100}; do echo "ITERATION $i"; PROVE_TESTS="t/019*" make -s check -C src/test/recovery/ || break; done
> > > > failed for me on iterations 2, 1, 7:
> > > > ITERATION 7
> > > > # +++ tap check in src/test/recovery +++
> > > > t/019_replslot_limit.pl .. 8/?
> > > > #   Failed test 'check that the slot state changes to "extended"'
> > > > #   at t/019_replslot_limit.pl line 140.
> > > > #          got: 'unreserved'
> > > > #     expected: 'extended'
> > > > t/019_replslot_limit.pl .. 26/? # Looks like you failed 1 test of 26.
> > > > t/019_replslot_limit.pl .. Dubious, test returned 1 (wstat 256, 0x100)
> > > > Failed 1/26 subtests
> > > >
> > > > With "WAIT FOR LSN" in wait_for_catchup() disabled, 100 iterations
> > > > passed.
> > > >
> > > > Having extra logging added, I could see the key difference.
> > > > Failed run:
> > > > 2026-05-19 22:01:37.968 EEST client backend[3632148] 019_replslot_limit.pl LOG:  !!!GetWALAvailability| targetLSN: 0/016C0000, targetSeg: 22, oldestSlotSeg: 23, oldestSegMaxWalSize: 24, oldestSeg: 22
> > > > 2026-05-19 22:01:37.968 EEST client backend[3632148] 019_replslot_limit.pl STATEMENT:  SELECT wal_status FROM pg_replication_slots WHERE slot_name = 'rep1'
> > > > vs
> > > > Successful run:
> > > > 2026-05-19 22:04:18.102 EEST client backend[3633761] 019_replslot_limit.pl LOG:  !!!GetWALAvailability| targetLSN: 0/01700000, targetSeg: 23, oldestSlotSeg: 23, oldestSegMaxWalSize: 24, oldestSeg: 23
> > > > 2026-05-19 22:04:18.102 EEST client backend[3633761] 019_replslot_limit.pl STATEMENT:  SELECT wal_status FROM pg_replication_slots WHERE slot_name = 'rep1'
> > > >
> > > > That is, with WAIT FOR LSN, primary in this test may advance
> > > > slot->data.restart_lsn to the expected position after wait_for_catchup()
> > > > returns.
> > > >
> > > > [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=goldfish&dt=2026-05-13%2006%3A15%3A03
> > >
> > > Thanks for reporting this issue.
> > >
> > > I think this is related to the semantic change made earlier:
> > > wait_for_catchup() now returns once the standby itself has reached the
> > > target LSN, rather than waiting until the primary observes that
> > > position via pg_stat_replication.
> > >
> > > As a result, the primary may not yet have processed the standby
> > > feedback needed to advance the slot's restart_lsn when
> > > wait_for_catchup() returns.
> > >
> > > Actually, I was aware of this semantic change and previously thought
> > > it might be harmless. But this failure appears to disprove that. I'll
> > > prepare a patch to fix this shortly.
> >
> > After some consideration, 019_replslot_limit.pl appears to the better
> > place to place the fix rather than by restoring the old primary-side
> > polling barrier in wait_for_catchup().
> >
> > The new wait_for_catchup() behavior is closer to its natural
> > semantics: for replay/write/flush modes, it waits until the standby
> > itself has reached the requested LSN. The old implementation used
> > pg_stat_replication on the primary, which also implied that the
> > primary had received and processed standby feedback. That was a useful
> > side effect for this test, but it is not required by most callers.
> >
> > 019_replslot_limit.pl is different because it checks primary-side slot
> > state. For a physical slot, restart_lsn advances only after the
> > primary's walsender processes standby feedback. So the test needs an
> > extra condition beyond ordinary standby catchup.
> >
> > The patch makes that dependency explicit: wait for the standby to
> > replay the target LSN, then wait for the slot's restart_lsn on the
> > primary to pass the same LSN. This keeps wait_for_catchup() focused on
> > standby catchup while making the slot-specific synchronization visible
> > in the test that needs it.
>
> I agree with you.  But do we actually need a
> wait_for_standby_and_slot_catchup() wrapper.  I think we can call
> $node->wait_for_slot_catchup() directly and simplify the fix.  Check
> the attached patch.
>

The patch looks good to me. I agree that the wait_for_slot_catchup is
not needed and could be misleading. This change would make the exact
synchronization point and its intention clearer. The only price we
need to pay here is bringing back the polling. But it seems acceptable
since the cost was there in the pre-wait-for-lsn era. And thanks for
writing the great commit message!

-- 
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

  85. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2026-05-23T18:40:30Z

    >
>
> > I agree with you.  But do we actually need a
> > wait_for_standby_and_slot_catchup() wrapper.  I think we can call
> > $node->wait_for_slot_catchup() directly and simplify the fix.  Check
> > the attached patch.
> >
>
> The patch looks good to me. I agree that the wait_for_slot_catchup is
> not needed and could be misleading. This change would make the exact
> synchronization point and its intention clearer. The only price we
> need to pay here is bringing back the polling. But it seems acceptable
> since the cost was there in the pre-wait-for-lsn era. And thanks for
> writing the great commit message!
>

Sorry for copy-pasting the wrong function name. It should be
wait_for_catchup().

>

Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

  86. Re: Implement waiting for wal lsn replay: reloaded

    Alexander Korotkov <aekorotkov@gmail.com> — 2026-05-25T09:00:20Z

    On Sat, May 23, 2026 at 9:40 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>> > I agree with you.  But do we actually need a
>> > wait_for_standby_and_slot_catchup() wrapper.  I think we can call
>> > $node->wait_for_slot_catchup() directly and simplify the fix.  Check
>> > the attached patch.
>> >
>>
>> The patch looks good to me. I agree that the wait_for_slot_catchup is
>> not needed and could be misleading. This change would make the exact
>> synchronization point and its intention clearer. The only price we
>> need to pay here is bringing back the polling. But it seems acceptable
>> since the cost was there in the pre-wait-for-lsn era. And thanks for
>> writing the great commit message!
>
>
> Sorry for copy-pasting the wrong function name. It should be wait_for_catchup().

Good, thank you.  I'll push it if no objections.

------
Regards,
Alexander Korotkov
Supabase

  87. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2026-05-26T01:48:26Z

    On Mon, May 25, 2026 at 5:00 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> On Sat, May 23, 2026 at 9:40 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >> > I agree with you.  But do we actually need a
> >> > wait_for_standby_and_slot_catchup() wrapper.  I think we can call
> >> > $node->wait_for_slot_catchup() directly and simplify the fix.  Check
> >> > the attached patch.
> >> >
> >>
> >> The patch looks good to me. I agree that the wait_for_slot_catchup is
> >> not needed and could be misleading. This change would make the exact
> >> synchronization point and its intention clearer. The only price we
> >> need to pay here is bringing back the polling. But it seems acceptable
> >> since the cost was there in the pre-wait-for-lsn era. And thanks for
> >> writing the great commit message!
> >
> >
> > Sorry for copy-pasting the wrong function name. It should be wait_for_catchup().
>
> Good, thank you.  I'll push it if no objections.

While reading 019_replslot_limit.pl, Codex pointed out a few
inconsistencies in the comments. I verified them and they look real.
Would you mind doing a small cleanup as well?

-- 
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

  88. Re: Implement waiting for wal lsn replay: reloaded

    Xuneng Zhou <xunengzhou@gmail.com> — 2026-05-26T15:53:14Z

    On Tue, May 26, 2026 at 9:48 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> On Mon, May 25, 2026 at 5:00 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >
> > On Sat, May 23, 2026 at 9:40 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > >> > I agree with you.  But do we actually need a
> > >> > wait_for_standby_and_slot_catchup() wrapper.  I think we can call
> > >> > $node->wait_for_slot_catchup() directly and simplify the fix.  Check
> > >> > the attached patch.
> > >> >
> > >>
> > >> The patch looks good to me. I agree that the wait_for_slot_catchup is
> > >> not needed and could be misleading. This change would make the exact
> > >> synchronization point and its intention clearer. The only price we
> > >> need to pay here is bringing back the polling. But it seems acceptable
> > >> since the cost was there in the pre-wait-for-lsn era. And thanks for
> > >> writing the great commit message!
> > >
> > >
> > > Sorry for copy-pasting the wrong function name. It should be wait_for_catchup().
> >
> > Good, thank you.  I'll push it if no objections.
>
> While reading 019_replslot_limit.pl, Codex pointed out a few
> inconsistencies in the comments. I verified them and they look real.
> Would you mind doing a small cleanup as well?

I updated the comment above wait_for_slot_catchup to reflect its usage.

-- 
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.