Re: Implement waiting for wal lsn replay: reloaded

Hi, Yura!

On Thu, Feb 6, 2025 at 10:31 AM Yura Sokolov <y(dot)sokolov(at)postgrespro(dot)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