Thread: Re: Implement waiting for wal lsn replay: reloaded
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
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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] 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