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

Jeff Davis <pgsql@j-davis.com>

From: Jeff Davis <pgsql@j-davis.com>

To: Andres Freund <andres@anarazel.de>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>

Cc: Dilip Kumar <dilipbalaut@gmail.com>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, pgsql-hackers@lists.postgresql.org, Nathan Bossart <nathandbossart@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>

Date: 2024-01-23T04:07:03Z

Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Add XLogCtl->logInsertResult
- f3ff7bf83bce 17.0 cited
Add assert to WALReadFromBuffers().
- 9ecbf54075a9 17.0 landed
Read WAL directly from WAL buffers.
- 91f2cae7a4e6 17.0 landed
Additional write barrier in AdvanceXLInsertBuffer().
- 766571be1659 17.0 landed
Use 64-bit atomics for xlblocks array elements.
- c3a8e2a7cb16 17.0 landed
Don't trust unvalidated xl_tot_len.
- bae868caf222 17.0 cited

Attachments

v19j-0001-Add-XLogReadFromBuffers.patch (text/x-patch) patch 0001
v19j-0002-Add-test-module-for-verifying-WAL-read-from-WAL.patch (text/x-patch) patch 0002

On Mon, 2024-01-22 at 12:12 -0800, Andres Freund wrote:
> I still think that anything that requires such checks shouldn't be
> merged. It's completely bogus to check page contents for validity
> when we
> should have metadata telling us which range of the buffers is valid
> and which
> not.

The check seems entirely unnecessary, to me. A leftover from v18?

I have attached a new patch (version "19j") to illustrate some of my
previous suggestions. I didn't spend a lot of time on it so it's not
ready for commit, but I believe my suggestions are easier to understand
in code form.

Note that, right now, it only works for XLogSendPhysical(). I believe
it's best to just make it work for 1-3 callers that we understand well,
and we can generalize later if it makes sense.

I'm still not clear on why some callers are reading XLOG_BLCKSZ
(expecting zeros at the end), and if it's OK to just change them to use
the exact byte count.

Also, if we've detected that the first requested buffer has been
evicted, is there any value in continuing the loop to see if more
recent buffers are available? For example, if the requested LSNs range
over buffers 4, 5, and 6, and 4 has already been evicted, should we try
to return LSN data from 5 and 6 at the proper offset in the dest
buffer? If so, we'd need to adjust the API so the caller knows what
parts of the dest buffer were filled in.

Regards,
	Jeff Davis