Implement waiting for wal lsn replay: reloaded

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. /messages/by-id/E1sZwuz-002NPQ-Lc@gemulon.postgresql.org
2. /messages/by-id/14de8671-e328-4c3e-b136-664f6f13a39f@iki.fi
3. /messages/by-id/CAPpHfdvRmTzGJw5rQdSMkTxUPZkjwtbQ=LJE2u9Jqh9gFXHpmg@mail.gmail.com
4. /messages/by-id/4953563546cb8c8851f84c7debf723ef@postgrespro.ru
5. /messages/by-id/ab0eddce-06d4-4db2-87ce-46fa2427806c@iki.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

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

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

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

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

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

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

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

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

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]/messages/by-id/09a98dc9-eeb1-471d-b990-072513c3d584@vondra.me are not yet addressed, I have
changed the commitfest status to Waiting on Author, please address
them and update it to Needs review.
[1]: /messages/by-id/09a98dc9-eeb1-471d-b990-072513c3d584@vondra.me

Regards,
Vignesh

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

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)

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]/messages/by-id/CABPTF7VuFYm9TtA9vY8ZtS77qsT+yL_HtSDxUFnW3XsdB5b9ew@mail.gmail.com 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]: /messages/by-id/CABPTF7VuFYm9TtA9vY8ZtS77qsT+yL_HtSDxUFnW3XsdB5b9ew@mail.gmail.com

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

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

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

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

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]/messages/by-id/CABPTF7Vr99gZ5GM_ZYbYnd9MMnoVW3pukBEviVoHKRvJW-dE3g@mail.gmail.com and attached a patch addressing the polling
issue in the function
read_local_xlog_page_guts built on the infra of patch v8.

[1]: /messages/by-id/CABPTF7Vr99gZ5GM_ZYbYnd9MMnoVW3pukBEviVoHKRvJW-dE3g@mail.gmail.com

Feedbacks welcome.

Best,
Xuneng

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] /messages/by-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. /messages/by-id/CABPTF7VsoGDMBq34MpLrMSZyxNZvVbgH6-zxtJOg5AwOoYURbw@mail.gmail.com

------
Regards,
Alexander Korotkov
Supabase