Subject: Re: Improve WALRead() to suck data directly from WAL buffers when possible

From: Jeff Davis <[email-masked]>
Date: Mon, 12 Feb 2024 11:33:24 -0800

On Wed, 2024-01-31 at 14:30 +0530, Bharath Rupireddy wrote:
> Please see the attached v22 patch set.

Committed 0001.

For 0002 & 0003, I'd like more clarity on how they will actually be
used by an extension.

For 0004, we need to resolve why callers are using XLOG_BLCKSZ and we
can fix that independently, as discussed here:

https://www.postgresql.org/message-id/CALj2ACV=C1GZT9XQRm4iN1NV1T=[email-masked]

Regards,
	Jeff Davis

---

From: Andres Freund <[email-masked]>
Date: Mon, 12 Feb 2024 12:18:53 -0800

Hi,

> > Please see the attached v22 patch set.

Yay, I think this is very cool. There are plenty other improvements than can
be based on this...

One thing I'm a bit confused in the code is the following:

+    /*
+     * Don't read past the available WAL data.
+     *
+     * Check using local copy of LogwrtResult. Ordinarily it's been updated by
+     * the caller when determining how far to read; but if not, it just means
+     * we'll read less data.
+     *
+     * XXX: the available WAL could be extended to the WAL insert pointer by
+     * calling WaitXLogInsertionsToFinish().
+     */
+    upto = Min(startptr + count, LogwrtResult.Write);
+    nbytes = upto - startptr;

Shouldn't it pretty much be a bug to ever encounter this? There aren't
equivalent checks in WALRead(), so any user of WALReadFromBuffers() that then
falls back to WALRead() is just going to send unwritten data.

ISTM that this should be an assertion or error.

Greetings,

Andres Freund

---

From: Jeff Davis <[email-masked]>
Date: Mon, 12 Feb 2024 12:46:00 -0800

On Mon, 2024-02-12 at 12:18 -0800, Andres Freund wrote:
> Shouldn't it pretty much be a bug to ever encounter this?

In the current code it's impossible, though Bharath hinted at an
extension which could reach that path.

What I committed was a bit of a compromise -- earlier versions of the
patch supported reading right up to the Insert pointer (which requires
a call to WaitXLogInsertionsToFinish()). I wasn't ready to commit that
code without seeing a more about how that would be used, but I thought
it was reasonable to have some simple code in there to allow reading up
to the Write pointer.

It seems closer to the structure that we will ultimately need to
replicate unflushed data, right?

Regards,
	Jeff Davis

[1]
https://www.postgresql.org/message-id/[email-masked]

---

From: Jeff Davis <[email-masked]>
Date: Mon, 12 Feb 2024 15:56:19 -0800

> For 0002 & 0003, I'd like more clarity on how they will actually be

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.

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.

Regards,
	Jeff Davis

---

From: Andres Freund <[email-masked]>
Date: Mon, 12 Feb 2024 16:11:50 -0800

Hi,

> 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.
>

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