Horton

Re: Implement waiting for wal lsn replay: reloaded

Alexander Korotkov <aekorotkov@gmail.com>

View thread
From: Alexander Korotkov <aekorotkov@gmail.com>
To: Yura Sokolov <y.sokolov@postgrespro.ru>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2025-02-16T21:27:43Z
Lists: pgsql-hackers

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

Attachments

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