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
