Hi, Yura!
On Thu, Feb 6, 2025 at 10:31 AM Yura Sokolov <y.soko...@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
v2-0001-Implement-WAIT-FOR-command.patch
Description: Binary data
