Hi, On 2024-02-12 15:56:19 -0800, Jeff Davis wrote: > On Mon, 2024-02-12 at 11:33 -0800, Jeff Davis wrote: > > For 0002 & 0003, I'd like more clarity on how they will actually be > > used by an extension. > > In patch 0002, I'm concerned about calling > WaitXLogInsertionsToFinish(). It loops through all the locks, but > doesn't have any early return path or advance any state.
I doubt it'd be too bad - we call that at much much higher frequency during write heavy OLTP workloads (c.f. XLogFlush()). It can be a performance issue there, but only after increasing NUM_XLOGINSERT_LOCKS - before that the limited number of writers is the limit. Compared to that walsender shouldn't be a significant factor. However, I think it's a very bad idea to call WALReadFromBuffers() from WALReadFromBuffers(). This needs to be at the caller, not down in WALReadFromBuffers(). I don't see why we would want to weaken the error condition in WaitXLogInsertionsToFinish() - I suspect it'd not work correctly to wait for insertions that aren't yet in progress and it just seems like an API misuse. > So if it's repeatedly called with the same or similar values it seems like > it would be doing a lot of extra work. > > I'm not sure of the best fix. We could add something to LogwrtResult to > track a new LSN that represents the highest known point where all > inserters are finished (in other words, the latest return value of > WaitXLogInsertionsToFinish()). That seems invasive, though. FWIW, I think LogwrtResult is an anti-pattern, perhaps introduced due to misunderstanding how cache coherency works. It's not fundamentally faster to access non-shared memory. It'd make far more sense to allow lock-free access to the shared LogwrtResult and Greetings, Andres Freund