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

From: Jeff Davis <[email-masked]>
Date: Tue, 03 Oct 2023 16:05:32 -0700

On Sat, 2023-01-14 at 12:34 -0800, Andres Freund wrote:
> One benefit would be that it'd make it more realistic to use direct
> IO for WAL
> - for which I have seen significant performance benefits. But when we
> afterwards have to re-read it from disk to replicate, it's less
> clearly a win.

Does this patch still look like a good fit for your (or someone else's)
plans for direct IO here? If so, would committing this soon make it
easier to make progress on that, or should we wait until it's actually
needed?

If I recall, this patch does not provide a perforance benefit as-is
(correct me if things have changed) and I don't know if a reduction in
syscalls alone is enough to justify it. But if it paves the way for
direct IO for WAL, that does seem worth it.

Regards,
	Jeff Davis

---

From: Andres Freund <[email-masked]>
Date: Wed, 11 Oct 2023 15:43:53 -0700

Hi,

> easier to make progress on that, or should we wait until it's actually

I think it'd be quite useful to have. Even with the code as of 16, I see
better performance in some workloads with debug_io_direct=wal,
wal_sync_method=open_datasync compared to any other configuration. Except of
course that it makes walsenders more problematic, as they suddenly require
read IO. Thus having support for walsenders to send directly from wal buffers
would be beneficial, even without further AIO infrastructure.

I also think there are other quite desirable features that are made easier by
this patch. One of the primary problems with using synchronous replication is
the latency increase, obviously. We can't send out WAL before it has locally
been wirten out and flushed to disk. For some workloads, we could
substantially lower synchronous commit latency if we were able to send WAL to
remote nodes *before* WAL has been made durable locally, even if the receiving
systems wouldn't be allowed to write that data to disk yet: It takes less time
to send just "write LSN: %X/%X, flush LSNL: %X/%X" than also having to send
all the not-yet-durable WAL.

In many OLTP workloads there won't be WAL flushes between generating WAL for
DML and commit, which means that the amount of WAL that needs to be sent out
at commit can be of nontrivial size.

E.g. for pgbench, normally a transaction is about ~550 bytes (fitting in a
single tcp/ip packet), but a pgbench transaction that needs to emit FPIs for
everything is a lot larger: ~45kB (not fitting in a single packet). Obviously
many real world workloads OLTP workloads actually do more writes than
pgbench. Making the commit latency of the latter be closer to the commit
latency of the former when using syncrep would obviously be great.

Of course this patch is just a relatively small step towards that: We'd also
need in-memory buffering on the receiving side, the replication protocol would
need to be improved, we'd likely need an option to explicitly opt into
receiving unflushed data. But it's still a pretty much required step.

Greetings,

Andres Freund

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 18 Oct 2023 01:32:54 +0530

> would be beneficial, even without further AIO infrastructure.

Right. Tests show the benefit with WAL DIO + this patch -
https://www.postgresql.org/message-id/CALj2ACV6rS%2B7iZx5%2BoAvyXJaN4AG-djAQeM1mrM%3DYSDkVrUs7g%40mail.gmail.com.

Also, irrespective of WAL DIO, the WAL buffers hit ratio with the
patch stood at 95% for 1 primary, 1 sync standby, 1 async standby,
pgbench --scale=300 --client=32 --time=900. In other words, the
walsenders avoided 95% of the time reading from the file/avoided pread
system calls - https://www.postgresql.org/message-id/CALj2ACXKKK%3DwbiG5_t6dGao5GoecMwRkhr7GjVBM_jg54%2BNa%3DQ%40mail.gmail.com.

> receiving unflushed data. But it's still a pretty much required step.

Yes, this patch can pave the way for all of the above features in
future. However, I'm looking forward to getting this in for now.
Later, I'll come up with more concrete thoughts on the above.

Having said above, the latest v10 patch after addressing some of the
review comments is at
https://www.postgresql.org/message-id/CALj2ACU3ZYzjOv4vZTR%2BLFk5PL4ndUnbLS6E1vG2dhDBjQGy2A%40mail.gmail.com.
Any further thoughts on the patch is welcome.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 20 Oct 2023 22:19:32 +0530

> would be beneficial, even without further AIO infrastructure.

I'm attaching the v11 patch set with the following changes:
- Improved input validation in the function that reads WAL from WAL
buffers in 0001 patch.
- Improved test module's code in 0002 patch.
- Modernized meson build file in 0002 patch.
- Added commit messages for both the patches.
- Ran pgindent on both the patches.

Any thoughts are welcome.

---

From: Bharath Rupireddy <[email-masked]>
Date: Sat, 21 Oct 2023 23:59:00 +0530

> Any thoughts are welcome.

I'm attaching v12 patch set with just pgperltidy ran on the new TAP
test added in 0002. No other changes from that of v11 patch set.

---

From: Jeff Davis <[email-masked]>
Date: Tue, 24 Oct 2023 17:15:19 -0700

> test added in 0002. No other changes from that of v11 patch set.

Thank you.

Comments:

* It would be good to document that this is partially an optimization
(read from memory first) and partially an API difference that allows
reading unflushed data. For instance, walsender may benefit
performance-wise (and perhaps later with the ability to read unflushed
data) whereas pg_walinspect benefits primarily from reading unflushed
data.

* Shouldn't there be a new method in XLogReaderRoutine (e.g.
read_unflushed_data), rather than having logic in WALRead()? The
callers can define the method if it makes sense (and that would be a
good place to document why); or leave it NULL if not.

* I'm not clear on the "partial hit" case. Wouldn't that mean you found
the earliest byte in the buffers, but not the latest byte requested? Is
that possible, and if so under what circumstances? I added an
"Assert(nread == 0 || nread == count)" in WALRead() after calling
XLogReadFromBuffers(), and it wasn't hit.

* If the partial hit case is important, wouldn't XLogReadFromBuffers()
fill in the end of the buffer rather than the beginning?

* Other functions that use xlblocks, e.g. GetXLogBuffer(), use more
effort to avoid acquiring WALBufMappingLock. Perhaps you can avoid it,
too? One idea is to check that XLogCtl->xlblocks[idx] is equal to
expectedEndPtr both before and after the memcpy(), with appropriate
barriers. That could mitigate concerns expressed by Kyotaro Horiguchi
and Masahiko Sawada.

* Are you sure that reducing the number of calls to memcpy() is a win?
I would expect that to be true only if the memcpy()s are tiny, but here
they are around XLOG_BLCKSZ. I believe this was done based on a comment
from Nathan Bossart, but I didn't really follow why that's important.
Also, if we try to use one memcpy for all of the data, it might not
interact well with my idea above to avoid taking the lock.

* Style-wise, the use of "unlikely" seems excessive, unless there's a
reason to think it matters.

Regards,
	Jeff Davis

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 24 Oct 2023 20:54:01 -0500

> interact well with my idea above to avoid taking the lock.

I don't recall exactly why I suggested this, but if additional memcpy()s
help in some way and don't negatively impact performance, then I retract my
previous comment.

-- 
Nathan Bossart

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 27 Oct 2023 03:46:32 +0530

> Comments:

Thanks for reviewing.

> data) whereas pg_walinspect benefits primarily from reading unflushed

Commit message has these things covered in detail. However, I think
adding some info in the code comments is a good idea and done around
the WALRead() function in the attached v13 patch set.

> good place to document why); or leave it NULL if not.

I've designed the new function XLogReadFromBuffers to read from WAL
buffers in such a way that one can easily embed it in page_read
callbacks if it makes sense. Almost all the available backend
page_read callbacks read_local_xlog_page_no_wait,
read_local_xlog_page, logical_read_xlog_page except XLogPageRead
(which is used for recovery when WAL buffers aren't used at all) have
one thing in common, that is, WALRead(). Therefore, it seemed a
natural choice for me to call XLogReadFromBuffers. In other words, I'd
say it's the responsibility of page_read callback implementers to
decide if they want to read from WAL buffers or not and hence I don't
think we need a separate XLogReaderRoutine.

If someone wants to read unflushed WAL, the typical way to implement
it is to write a new page_read callback
read_local_unflushed_xlog_page/logical_read_unflushed_xlog_page or
similar without WALRead() but just the new function
XLogReadFromBuffers to read from WAL buffers and return.

> fill in the end of the buffer rather than the beginning?

Partial hit was possible when the requested WAL pages are read one
page at a time from WAL buffers with WALBufMappingLock
acquisition-release for each page as the requested page can be
replaced by the time the lock is released and reacquired. This was the
case up until the v6 patch -
https://www.postgresql.org/message-id/CALj2ACWTNneq2EjMDyUeWF-BnwpewuhiNEfjo9bxLwFU9iPF0w%40mail.gmail.com.
Now that the approach has been changed to read multiple pages at once
under one WALBufMappingLock acquisition-release. .
We can either keep the partial hit handling (just to not miss
anything) or turn the following partial hit case to an error or an
Assert(false);. I prefer to keep the partial hit handling as-is just
in case:
+        else if (count > nread)
+        {
+            /*
+             * Buffer partial hit, so reset the state to count the read bytes
+             * and continue.
+             */
+            buf += nread;
+            startptr += nread;
+            count -= nread;
+        }

> barriers. That could mitigate concerns expressed by Kyotaro Horiguchi

Yes, I proposed that idea in another thread -
https://www.postgresql.org/message-id/CALj2ACVFSirOFiABrNVAA6JtPHvA9iu%2Bwp%3DqkM9pdLZ5mwLaFg%40mail.gmail.com.
If that looks okay, I can add it to the next version of this patch
set.

> interact well with my idea above to avoid taking the lock.

Up until the v6 patch -
https://www.postgresql.org/message-id/CALj2ACWTNneq2EjMDyUeWF-BnwpewuhiNEfjo9bxLwFU9iPF0w%40mail.gmail.com,
the requested WAL was being read one page at a time from WAL buffers
into output buffer with one memcpy call for each page. Now that the
approach has been changed to read multiple pages at once under one
WALBufMappingLock acquisition-release with comparatively lesser number
of memcpy calls. I honestly haven't seen any difference between the
two approaches -
https://www.postgresql.org/message-id/CALj2ACUpQGiwQTzmoSMOFk5%3DWiJc06FcYpxzBX0SEej4ProRzg%40mail.gmail.com.

The new approach of reading multiple pages at once under one
WALBufMappingLock acquisition-release clearly wins over reading one
page at a time with multiple lock  acquisition-release cycles.

> reason to think it matters.

Given the current use of XLogReadFromBuffers, the input parameters are
passed as expected, IOW, these are unlikely events. The comments [1]
say that the unlikely() is to be used in hot code paths; I think
reading WAL from buffers is a hot code path especially when called
from (logical, physical) walsenders. If there's any stronger reason
than the appearance/style-wise, I'm okay to not use them. For now,
I've retained them.

FWIW, I found heapam.c using unlikely() extensively for safety checks.

[1]
 * Hints to the compiler about the likelihood of a branch. Both likely() and
 * unlikely() return the boolean value of the contained expression.
 *
 * These should only be used sparingly, in very hot code paths. It's very easy
 * to mis-estimate likelihoods.

---

From: Jeff Davis <[email-masked]>
Date: Fri, 27 Oct 2023 13:52:56 -0700

On Fri, 2023-10-27 at 03:46 +0530, Bharath Rupireddy wrote:
> 
> Almost all the available backend

I think I see what you are saying: WALRead() is at a lower level than
the XLogReaderRoutine callbacks, because it's used by the .page_read
callbacks.

That makes sense, but my first interpretation was that WALRead() is
above the XLogReaderRoutine callbacks because it calls .segment_open
and .segment_close. To me that sounds like a layering violation, but it
exists today without your patch.

I suppose the question is: should reading from the WAL buffers an
intentional thing that the caller does explicitly by specific callers?
Or is it an optimization that should be hidden from the caller?

I tend toward the former, at least for now. I suspect that when we do
some more interesting things, like replicating unflushed data, we will
want reading from buffers to be a separate step, not combined with
WALRead(). After things in this area settle a bit then we might want to
refactor and combine them again.

> XLogReadFromBuffers to read from WAL buffers and return.

Then why is it being called from WALRead() at all?

> 
> I prefer to keep the partial hit handling as-is just
> in case:
> 

So a "partial hit" is essentially a narrow race condition where one
page is read from buffers, and it's valid; and by the time it gets to
the next page, it has already been evicted (along with the previously
read page)? In other words, I think you are describing a case where
eviction is happening faster than the memcpy()s in a loop, which is
certainly possible due to scheduling or whatever, but doesn't seem like
the common case.

The case I'd expect for a partial read is when the startptr points to
an evicted page, but some later pages in the requested range are still
present in the buffers.

I'm not really sure whether either of these cases matters, but if we
implement one and not the other, there should be some explanation.

> If that looks okay, I can add it to the next version of this patch

The code in the email above still shows a call to:

  /*
   * Requested WAL is available in WAL buffers, so recheck the
existence
   * under the WALBufMappingLock and read if the page still exists,
otherwise
   * return.
   */
  LWLockAcquire(WALBufMappingLock, LW_SHARED);

and I don't think that's required. How about something like:

  endptr1 = XLogCtl->xlblocks[idx];
  /* Requested WAL isn't available in WAL buffers. */
  if (expectedEndPtr != endptr1)
      break;

  pg_read_barrier();
  ...
  memcpy(buf, data, bytes_read_this_loop);
  ...
  pg_read_barrier();
  endptr2 = XLogCtl->xlblocks[idx];
  if (expectedEndPtr != endptr2)
      break;

  ntotal += bytes_read_this_loop;
  /* success; move on to next page */

I'm not sure why GetXLogBuffer() doesn't just use pg_atomic_read_u64().
I suppose because xlblocks are not guaranteed to be 64-bit aligned?
Should we just align it to 64 bits so we can use atomics? (I don't
think it matters in this case, but atomics would avoid the need to
think about it.)

> FWIW, I found heapam.c using unlikely() extensively for safety

OK, I won't object to the use of unlikely(), though I typically don't
use it without a fairly strong reason to think I should override what
the compiler thinks and/or what branch predictors can handle.

In this case, I think some of those errors are not really necessary
anyway, though:

  * XLogReadFromBuffers shouldn't take a timeline argument just to
demand that it's always equal to the wal insertion timeline.
  * Why check that startptr is earlier than the flush pointer, but not
startptr+count? Also, given that we intend to support reading unflushed
data, it would be good to comment that the function still works past
the flush pointer, and that it will be safe to remove later (right?).
  * An "Assert(!RecoveryInProgress())" would be more appropriate than
an error. Perhaps we will remove even that check in the future to
achieve cascaded replication of unflushed data.

Regards,
	Jeff Davis

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 2 Nov 2023 22:38:38 +0530

> exists today without your patch.

Right. WALRead() is a common function used by most if not all
page_read callbacks. Typically, the page_read callbacks code has 2
parts - first determine the target/start LSN and second read WAL (via
WALRead() for instance).

> I tend toward the former, at least for now.

Yes, it's an optimization that must be hidden from the caller.

> I suspect that when we do

As said previously, the new XLogReadFromBuffers() function is generic
and extensible in the way that anyone with a target/start LSN
(corresponding to flushed or written-but-not-yet-flushed WAL) and TLI
can call it to read from WAL buffers. It's just that the patch
currently uses it where it makes sense i.e. in WALRead(). But, it's
usable in, say, a page_read callback reading unflushed WAL from WAL
buffers.

> Then why is it being called from WALRead() at all?

The patch focuses on reading flushed WAL from WAL buffers if
available, not the unflushed WAL at all; that's why it's in WALRead()
before reading from the WAL file using pg_pread().

I'm trying to make a point that the XLogReadFromBuffers() enables one
to read unflushed WAL from WAL buffers (if really wanted for future
features like replicate from WAL buffers as a new opt-in feature to
improve the replication performance).

> read page)?
> In other words, I think you are describing a case where

At any given point of time, WAL buffer pages are maintained as a
circularly sorted array in an ascending order from
OldestInitializedPage to InitializedUpTo (new pages are inserted at
this end). Also, the current patch holds WALBufMappingLock while
reading the buffer pages, meaning, no one can replace the buffer pages
until reading is finished. Therefore, the above two described partial
hit cases can't happen - when reading multiple pages if the first page
is found to be existing in the buffer pages, it means the other pages
must exist too because of the circular and sortedness of the WAL
buffer page array.

Here's an illustration with WAL buffers circular array (idx, LSN) of
size 10 elements with contents as {(0, 160), (1, 170), (2, 180), (3,
90), (4, 100), (5, 110), (6, 120), (7, 130), (8, 140), (9, 150)} and
current InitializedUpTo pointing to page at LSN 180, idx 2.
- Read 6 pages starting from LSN 80. Nothing is read from WAL buffers
as the page at LSN 80 doesn't exist despite other 5 pages starting
from LSN 90 exist.
- Read 6 pages starting from LSN 90. All the pages exist and are read
from WAL buffers.
- Read 6 pages starting from LSN 150. Note that WAL is currently
flushed only up to page at LSN 180 and the callers won't ask for
unflushed WAL read. If a caller asks for an unflushed WAL read
intentionally or unintentionally, XLogReadFromBuffers() reads only 4
pages starting from LSN 150 to LSN 180 and will leave the remaining 2
pages for the caller to deal with. This is the partial hit that can
happen. Therefore, retaining the partial hit code in WALRead() as-is
in the current patch is needed IMV.

> think it matters in this case, but atomics would avoid the need to

WALBufMappingLock protects both xlblocks and WAL buffer pages [1][2].
I'm not sure how using the memory barrier, not WALBufMappingLock,
prevents writers from replacing WAL buffer pages while readers reading
the pages. FWIW, GetXLogBuffer() reads the xlblocks value without the
lock but it confirms the WAL existence under the lock and gets the WAL
buffer page under the lock [3].

I'll reiterate the WALBufMappingLock thing for this patch - the idea
is to know whether or not the WAL at a given LSN exists in WAL buffers
without acquiring WALBufMappingLock; if exists acquire the lock in
shared mode, read from WAL buffers and then release. WAL buffer pages
are organized as a circular array with the InitializedUpTo as the
latest filled WAL buffer page. If there's a way to track the oldest
filled WAL buffer page (OldestInitializedPage), at any given point of
time, the elements of the circular array are sorted in an ascending
order from OldestInitializedPage to InitializedUpTo. With this
approach, no lock is required to know if the WAL at given LSN exists
in WAL buffers, we can just do this if lsn >=
XLogCtl->OldestInitializedPage && lsn < XLogCtl->InitializedUpTo. I
proposed this idea here
https://www.postgresql.org/message-id/CALj2ACVgi6LirgLDZh%3DFdfdvGvKAD%3D%3DWTOSWcQy%3DAtNgPDVnKw%40mail.gmail.com.
I've pulled that patch in here as 0001 to showcase its use for this
feature.

> the flush pointer, and that it will be safe to remove later (right?).

That won't work, see the comment below. Actual flushed LSN may not
always be greater than startptr+count. GetFlushRecPtr() check in
XLogReadFromBuffers() is similar to what pg_walinspect has in
GetCurrentLSN().

    /*
     * Even though we just determined how much of the page can be validly read
     * as 'count', read the whole page anyway. It's guaranteed to be
     * zero-padded up to the page boundary if it's incomplete.
     */
    if (!WALRead(state, cur_page, targetPagePtr, XLOG_BLCKSZ, tli,
                 &errinfo))

> demand that it's always equal to the wal insertion timeline.

I've changed XLogReadFromBuffers() to return as-if nothing was read
(cache miss) when the server is in recovery or the requested TLI is
not the current server's insertion TLI. It is better than failing with
ERRORs so that  the callers don't have to have any checks for recovery
or TLI.

PSA v14 patch set.

[1]
 * WALBufMappingLock: must be held to replace a page in the WAL buffer cache.

[2]
     * and xlblocks values certainly do.  xlblocks values are protected by
     * WALBufMappingLock.
     */
    char       *pages;            /* buffers for unwritten XLOG pages */
    XLogRecPtr *xlblocks;        /* 1st byte ptr-s + XLOG_BLCKSZ */

[3]
     * However, we don't hold a lock while we read the value. If someone has
     * just initialized the page, it's possible that we get a "torn read" of
     * the XLogRecPtr if 64-bit fetches are not atomic on this platform. In
     * that case we will see a bogus value. That's ok, we'll grab the mapping
     * lock (in AdvanceXLInsertBuffer) and retry if we see anything else than
     * the page we're looking for. But it means that when we do this unlocked
     * read, we might see a value that appears to be ahead of the page we're
     * looking for. Don't PANIC on that, until we've verified the value while
     * holding the lock.
     */
    expectedEndPtr = ptr;
    expectedEndPtr += XLOG_BLCKSZ - ptr % XLOG_BLCKSZ;

    endptr = XLogCtl->xlblocks[idx];

---

From: Jeff Davis <[email-masked]>
Date: Fri, 03 Nov 2023 00:05:06 -0700

> Yes, it's an optimization that must be hidden from the caller.

As I said, I tend toward the opposite: that specific callers should
read from the buffers explicitly in the cases where it makes sense.

I don't think this is the most important point right now though, let's
sort out the other details.

> this end).

I don't see any reference to OldestInitializedPage or anything like it,
with or without your patch. Am I missing something?

> as the page at LSN 80 doesn't exist despite other 5 pages starting

This is what I imagined a "partial hit" was: read the 5 pages starting
at 90. The caller would then need to figure out how to read the page at
LSN 80 from the segment files.

I am not saying we should support this case; perhaps it doesn't matter.
I'm just describing why that term was confusing for me.

> If a caller asks for an unflushed WAL read
> happen.

To me that's more like an EOF case. "Partial hit" sounds to me like the
data exists but is not available in the cache (i.e. go to the segment
files); whereas if it encountered the end, the data is not available at
all.

> the pages.

It doesn't *prevent* that case, but it does *detect* that case. We
don't want to prevent writers from replacing WAL buffers, because that
would mean we are slowing down the critical WAL writing path.

Let me explain the potential problem cases, and how the barriers
prevent them:

Potential problem 1: the page is not yet resident in the cache at the
time the memcpy begins. In this case, the first read barrier would
ensure that the page is also not yet resident at the time xlblocks[idx]
is read into endptr1, and we'd break out of the loop.

Potential problem 2: the page is evicted before the memcpy finishes. In
this case, the second read barrier would ensure that the page was also
evicted before xlblocks[idx] is read into endptr2, and again we'd
detect the problem and break out of the loop.

I assume here that, if xlblocks[idx] holds the endPtr of the desired
page, all of the bytes for that page are resident at that moment. I
don't think that's true right now: AdvanceXLInsertBuffers() zeroes the
old page before updating xlblocks[nextidx]. I think it needs something
like:

  pg_atomic_write_u64(&XLogCtl->xlblocks[nextidx], InvalidXLogRecPtr);
  pg_write_barrier();

before the MemSet.

I didn't review your latest v14 patch yet.

Regards,
	Jeff Davis

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 3 Nov 2023 20:23:30 +0530

> read from the buffers explicitly in the cases where it makes sense.

How about adding a bool flag (read_from_wal_buffers) to
XLogReaderState so that the callers can set it if they want this
facility via XLogReaderAllocate()?

> with or without your patch. Am I missing something?

OldestInitializedPage is introduced in v14-0001 patch. Please have a look.

> I'm just describing why that term was confusing for me.

Okay. Current patch doesn't support this case.

> files); whereas if it encountered the end, the data is not available at

Right. We can tweak the comments around "partial hit" if required.

> detect the problem and break out of the loop.

Understood.

> old page before updating xlblocks[nextidx].

Right.

> I think it needs something like:
>

I think it works. First, xlblocks needs to be turned to an array of
64-bit atomics and then the above change. With this, all those who
reads xlblocks with or without WALBufMappingLock also need to check if
xlblocks[idx] is ever InvalidXLogRecPtr and take appropriate action.

I'm sure you have seen the following. It looks like I'm leaning
towards the claim that it's safe to read xlblocks without
WALBufMappingLock. I'll put up a patch for these changes separately.

        /*
         * Make sure the initialization of the page becomes visible to others
         * before the xlblocks update. GetXLogBuffer() reads xlblocks without
         * holding a lock.
         */
        pg_write_barrier();

        *((volatile XLogRecPtr *) &XLogCtl->xlblocks[nextidx]) = NewPageEndPtr;

I think the 3 things that helps read from WAL buffers without
WALBufMappingLock are: 1) couple of the read barriers in
XLogReadFromBuffers, 2) atomically initializing xlblocks[idx] to
InvalidXLogRecPtr plus a write barrier in AdvanceXLInsertBuffer(), 3)
the following sanity check to see if the read page is valid in
XLogReadFromBuffers(). If it sounds sensible, I'll work towards coding
it up. Thoughts?

+ , we
+         * need to ensure that we are not reading a page that just got
+         * initialized. For this, we look at the needed page header.
+         */
+        phdr = (XLogPageHeader) page;
+
+        /* Return, if WAL buffer page doesn't look valid. */
+        if (!(phdr->xlp_magic == XLOG_PAGE_MAGIC &&
+              phdr->xlp_pageaddr == (ptr - (ptr % XLOG_BLCKSZ)) &&
+              phdr->xlp_tli == tli))
+            break;
+

---

From: Jeff Davis <[email-masked]>
Date: Fri, 03 Nov 2023 12:47:24 -0700

> OldestInitializedPage is introduced in v14-0001 patch. Please have a

I don't see why that's necessary if we move to the algorithm I
suggested below that doesn't require a lock.

> > 
> Okay. Current patch doesn't support this [partial hit of newer pages]
> case.

OK, no need to support it until you see a reason.

> 64-bit atomics and then the above change.

Does anyone see a reason we shouldn't move to atomics here?

>         *((volatile XLogRecPtr *) &XLogCtl->xlblocks[nextidx]) =

I am confused why the "volatile" is required on that line (not from
your patch). I sent a separate message about that:

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

> XLogReadFromBuffers(). If it sounds sensible, I'll work towards

I like it. I think it will ultimately be a fairly simple loop. And by
moving to atomics, we won't need the delicate comment in
GetXLogBuffer().

Regards,
	Jeff Davis

---

From: Andres Freund <[email-masked]>
Date: Fri, 3 Nov 2023 16:58:18 -0700

Hi,

> facility via XLogReaderAllocate()?

That seems wrong architecturally - why should xlogreader itself know about any
of this? What would it mean in frontend code if read_from_wal_buffers were
set? IMO this is something that should happen purely within the read function.

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Fri, 3 Nov 2023 17:43:48 -0700

Hi,

On 2023-11-02 22:38:38 +0530, Bharath Rupireddy wrote:
> From 5b5469d7dcd8e98bfcaf14227e67356bbc1f5fe8 Mon Sep 17 00:00:00 2001
> From: Bharath Rupireddy <[email-masked]>
> Date: Thu, 2 Nov 2023 15:10:51 +0000
> Subject: [PATCH v14] Track oldest initialized WAL buffer page
>
> ---
>  src/backend/access/transam/xlog.c | 170 ++++++++++++++++++++++++++++++
>  src/include/access/xlog.h         |   1 +
>  2 files changed, 171 insertions(+)
>
> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
> index b541be8eec..fdf2ef310b 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -504,6 +504,45 @@ typedef struct XLogCtlData
>  	int			XLogCacheBlck;	/* highest allocated xlog buffer index */
>
> +	/*
> +	 * Start address of oldest initialized page in XLog buffers.
> +	 *
> +	 * We mainly track oldest initialized page explicitly to quickly tell if a
> +	 * given WAL record is available in XLog buffers. It also can be used for
> +	 * other purposes, see notes below.
> +	 *
> +	 * OldestInitializedPage gives XLog buffers following properties:
> +	 *
> +	 * 1) At any given point of time, pages in XLog buffers array are sorted
> +	 * in an ascending order from OldestInitializedPage till InitializedUpTo.
> +	 * Note that we verify this property for assert-only builds, see
> +	 * IsXLogBuffersArraySorted() for more details.

This is true - but also not, if you look at it a bit too literally. The
buffers in xlblocks itself obviously aren't ordered when wrapping around
between XLogRecPtrToBufIdx(OldestInitializedPage) and
XLogRecPtrToBufIdx(InitializedUpTo).

> +	 * 2) OldestInitializedPage is monotonically increasing (by virtue of how
> +	 * postgres generates WAL records), that is, its value never decreases.
> +	 * This property lets someone read its value without a lock. There's no
> +	 * problem even if its value is slightly stale i.e. concurrently being
> +	 * updated. One can still use it for finding if a given WAL record is
> +	 * available in XLog buffers. At worst, one might get false positives
> +	 * (i.e. OldestInitializedPage may tell that the WAL record is available
> +	 * in XLog buffers, but when one actually looks at it, it isn't really
> +	 * available). This is more efficient and performant than acquiring a lock
> +	 * for reading. Note that we may not need a lock to read
> +	 * OldestInitializedPage but we need to update it holding
> +	 * WALBufMappingLock.

I'd
s/may not need/do not need/

But perhaps rephrase it a bit more, to something like:

To update OldestInitializedPage, WALBufMappingLock needs to be held
exclusively, for reading no lock is required.

> +	 *
> +	 * 3) One can start traversing XLog buffers from OldestInitializedPage
> +	 * till InitializedUpTo to list out all valid WAL records and stats, and
> +	 * expose them via SQL-callable functions to users.
> +	 *
> +	 * 4) XLog buffers array is inherently organized as a circular, sorted and
> +	 * rotated array with OldestInitializedPage as pivot with the property
> +	 * where LSN of previous buffer page (if valid) is greater than
> +	 * OldestInitializedPage and LSN of next buffer page (if valid) is greater
> +	 * than OldestInitializedPage.
> +	 */
> +	XLogRecPtr	OldestInitializedPage;

It seems a bit odd to name a LSN containing variable *Page...

>  	/*
>  	 * InsertTimeLineID is the timeline into which new WAL is being inserted
>  	 * and flushed. It is zero during recovery, and does not change once set.
> @@ -590,6 +629,10 @@ static ControlFileData *ControlFile = NULL;
>  #define NextBufIdx(idx)		\
>  		(((idx) == XLogCtl->XLogCacheBlck) ? 0 : ((idx) + 1))
>
> +/* Macro to retreat to previous buffer index. */
> +#define PreviousBufIdx(idx)		\
> +		(((idx) == 0) ? XLogCtl->XLogCacheBlck : ((idx) - 1))

I think it might be worth making these inlines and adding assertions that idx
is not bigger than XLogCtl->XLogCacheBlck?

>  /*
>   * XLogRecPtrToBufIdx returns the index of the WAL buffer that holds, or
>   * would hold if it was in cache, the page containing 'recptr'.
> @@ -708,6 +751,10 @@ static void WALInsertLockAcquireExclusive(void);
>  static void WALInsertLockRelease(void);
>  static void WALInsertLockUpdateInsertingAt(XLogRecPtr insertingAt);
>
> +#ifdef USE_ASSERT_CHECKING
> +static bool IsXLogBuffersArraySorted(void);
> +#endif
> +
>  /*
>   * Insert an XLOG record represented by an already-constructed chain of data
>   * chunks.  This is a low-level routine; to construct the WAL record header
> @@ -1992,6 +2039,52 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic)
>  		XLogCtl->InitializedUpTo = NewPageEndPtr;
>
>  		npages++;
> +
> +		/*
> +		 * Try updating oldest initialized XLog buffer page.
> +		 *
> +		 * Update it if we are initializing an XLog buffer page for the first
> +		 * time or if XLog buffers are full and we are wrapping around.
> +		 */
> +		if (XLogRecPtrIsInvalid(XLogCtl->OldestInitializedPage) ||
> +			XLogRecPtrToBufIdx(XLogCtl->OldestInitializedPage) == nextidx)
> +		{
> +			Assert(XLogCtl->OldestInitializedPage < NewPageBeginPtr);
> +
> +			XLogCtl->OldestInitializedPage = NewPageBeginPtr;
> +		}

Wait, isn't this too late?  At this point the buffer can already be used by
GetXLogBuffers().  I think thi sneeds to happen at the latest just before
		*((volatile XLogRecPtr *) &XLogCtl->xlblocks[nextidx]) = NewPageEndPtr;

Why is it legal to get here with XLogCtl->OldestInitializedPage being invalid?

> +
> +/*
> + * Returns whether or not a given WAL record is available in XLog buffers.
> + *
> + * Note that we don't read OldestInitializedPage under a lock, see description
> + * near its definition in xlog.c for more details.
> + *
> + * Note that caller needs to pass in an LSN known to the server, not a future
> + * or unwritten or unflushed LSN.
> + */
> +bool
> +IsWALRecordAvailableInXLogBuffers(XLogRecPtr lsn)
> +{
> +	if (!XLogRecPtrIsInvalid(lsn) &&
> +		!XLogRecPtrIsInvalid(XLogCtl->OldestInitializedPage) &&
> +		lsn >= XLogCtl->OldestInitializedPage &&
> +		lsn < XLogCtl->InitializedUpTo)
> +	{
> +		return true;
> +	}
> +
> +	return false;
> +}
> diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
> index a14126d164..35235010e6 100644
> --- a/src/include/access/xlog.h
> +++ b/src/include/access/xlog.h
> @@ -261,6 +261,7 @@ extern void ReachedEndOfBackup(XLogRecPtr EndRecPtr, TimeLineID tli);
>  extern void SetInstallXLogFileSegmentActive(void);
>  extern bool IsInstallXLogFileSegmentActive(void);
>  extern void XLogShutdownWalRcv(void);
> +extern bool IsWALRecordAvailableInXLogBuffers(XLogRecPtr lsn);
>
>  /*
>   * Routines to start, stop, and get status of a base backup.
> --
> 2.34.1
>

> From db027d8f1dcb53ebceef0135287f120acf67cc21 Mon Sep 17 00:00:00 2001
> From: Bharath Rupireddy <[email-masked]>
> Date: Thu, 2 Nov 2023 15:36:11 +0000
> Subject: [PATCH v14] Allow WAL reading from WAL buffers
>
> This commit adds WALRead() the capability to read WAL from WAL
> buffers when possible. When requested WAL isn't available in WAL
> buffers, the WAL is read from the WAL file as usual. It relies on
> WALBufMappingLock so that no one replaces the WAL buffer page that
> we're reading from. It skips reading from WAL buffers if
> WALBufMappingLock can't be acquired immediately. In other words,
> it doesn't wait for WALBufMappingLock to be available. This helps
> reduce the contention on WALBufMappingLock.
>
> This commit benefits the callers of WALRead(), that are walsenders
> and pg_walinspect. They can now avoid reading WAL from the WAL
> file (possibly avoiding disk IO). Tests show that the WAL buffers
> hit ratio stood at 95% for 1 primary, 1 sync standby, 1 async
> standby, with pgbench --scale=300 --client=32 --time=900. In other
> words, the walsenders avoided 95% of the time reading from the
> file/avoided pread system calls:
> https://www.postgresql.org/message-id/CALj2ACXKKK%3DwbiG5_t6dGao5GoecMwRkhr7GjVBM_jg54%2BNa%3DQ%40mail.gmail.com
>
> This commit also benefits when direct IO is enabled for WAL.
> Reading WAL from WAL buffers puts back the performance close to
> that of without direct IO for WAL:
> https://www.postgresql.org/message-id/CALj2ACV6rS%2B7iZx5%2BoAvyXJaN4AG-djAQeM1mrM%3DYSDkVrUs7g%40mail.gmail.com
>
> This commit also paves the way for the following features in
> future:
> - Improves synchronous replication performance by replicating
> directly from WAL buffers.
> - A opt-in way for the walreceivers to receive unflushed WAL.
> More details here:
> https://www.postgresql.org/message-id/20231011224353.cl7c2s222dw3de4j%40awork3.anarazel.de
>
> Author: Bharath Rupireddy
> Reviewed-by: Dilip Kumar, Andres Freund
> Reviewed-by: Nathan Bossart, Kuntal Ghosh
> Discussion: https://www.postgresql.org/message-id/CALj2ACXKKK%3DwbiG5_t6dGao5GoecMwRkhr7GjVBM_jg54%2BNa%3DQ%40mail.gmail.com
> ---
>  src/backend/access/transam/xlog.c       | 205 ++++++++++++++++++++++++
>  src/backend/access/transam/xlogreader.c |  41 ++++-
>  src/include/access/xlog.h               |   6 +
>  3 files changed, 250 insertions(+), 2 deletions(-)
>
> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
> index fdf2ef310b..ff5dccaaa7 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -1753,6 +1753,211 @@ GetXLogBuffer(XLogRecPtr ptr, TimeLineID tli)
>  	return cachedPos + ptr % XLOG_BLCKSZ;
>  }

> +/*
> + * Read WAL from WAL buffers.
> + *
> + * Read 'count' bytes of WAL from WAL buffers into 'buf', starting at location
> + * 'startptr', on timeline 'tli' and return total read bytes.
> + *
> + * This function returns quickly in the following cases:
> + * - When passed-in timeline is different than server's current insertion
> + * timeline as WAL is always inserted into WAL buffers on insertion timeline.
> + * - When server is in recovery as WAL buffers aren't currently used in
> + * recovery.
> + *
> + * Note that this function reads as much as it can from WAL buffers, meaning,
> + * it may not read all the requested 'count' bytes. Caller must be aware of
> + * this and deal with it.
> + *
> + * Note that this function is not available for frontend code as WAL buffers is
> + * an internal mechanism to the server.
>
> + */
> +Size
> +XLogReadFromBuffers(XLogReaderState *state,
> +					XLogRecPtr startptr,
> +					TimeLineID tli,
> +					Size count,
> +					char *buf)
> +{
> +	XLogRecPtr	ptr;
> +	XLogRecPtr	cur_lsn;
> +	Size		nbytes;
> +	Size		ntotal;
> +	Size		nbatch;
> +	char	   *batchstart;
> +
> +	if (RecoveryInProgress())
> +		return 0;
>
> +	if (tli != GetWALInsertionTimeLine())
> +		return 0;
> +
> +	Assert(!XLogRecPtrIsInvalid(startptr));
> +
> +	cur_lsn = GetFlushRecPtr(NULL);
> +	if (unlikely(startptr > cur_lsn))
> +		elog(ERROR, "WAL start LSN %X/%X specified for reading from WAL buffers must be less than current database system WAL LSN %X/%X",
> +			 LSN_FORMAT_ARGS(startptr), LSN_FORMAT_ARGS(cur_lsn));

Hm, why does this check belong here? For some tools it might be legitimate to
read the WAL before it was fully flushed.

> +	/*
> +	 * Holding WALBufMappingLock ensures inserters don't overwrite this value
> +	 * while we are reading it. We try to acquire it in shared mode so that
> +	 * the concurrent WAL readers are also allowed. We try to do as less work
> +	 * as possible while holding the lock to not impact concurrent WAL writers
> +	 * much. We quickly exit to not cause any contention, if the lock isn't
> +	 * immediately available.
> +	 */
> +	if (!LWLockConditionalAcquire(WALBufMappingLock, LW_SHARED))
> +		return 0;

That seems problematic - that lock is often heavily contended.  We could
instead check IsWALRecordAvailableInXLogBuffers() once before reading the
page, then read the page contents *without* holding a lock, and then check
IsWALRecordAvailableInXLogBuffers() again - if the page was replaced in the
interim we read bogus data, but that's a bit of a wasted effort.

> +	ptr = startptr;
> +	nbytes = count;				/* Total bytes requested to be read by caller. */
> +	ntotal = 0;					/* Total bytes read. */
> +	nbatch = 0;					/* Bytes to be read in single batch. */
> +	batchstart = NULL;			/* Location to read from for single batch. */

What does "batch" mean?

> +	while (nbytes > 0)
> +	{
> +		XLogRecPtr	expectedEndPtr;
> +		XLogRecPtr	endptr;
> +		int			idx;
> +		char	   *page;
> +		char	   *data;
> +		XLogPageHeader phdr;
> +
> +		idx = XLogRecPtrToBufIdx(ptr);
> +		expectedEndPtr = ptr;
> +		expectedEndPtr += XLOG_BLCKSZ - ptr % XLOG_BLCKSZ;
> +		endptr = XLogCtl->xlblocks[idx];
> +
> +		/* Requested WAL isn't available in WAL buffers. */
> +		if (expectedEndPtr != endptr)
> +			break;
> +
> +		/*
> +		 * We found WAL buffer page containing given XLogRecPtr. Get starting
> +		 * address of the page and a pointer to the right location of given
> +		 * XLogRecPtr in that page.
> +		 */
> +		page = XLogCtl->pages + idx * (Size) XLOG_BLCKSZ;
> +		data = page + ptr % XLOG_BLCKSZ;
> +
> +		/*
> +		 * The fact that we acquire WALBufMappingLock while reading the WAL
> +		 * buffer page itself guarantees that no one else initializes it or
> +		 * makes it ready for next use in AdvanceXLInsertBuffer(). However, we

I don't think this code should ever encounter a page where this is not the
case?  We particularly shouldn't do so silently, seems that could hide all
kinds of problems.

> +		/*
> +		 * Note that we don't perform all page header checks here to avoid
> +		 * extra work in production builds; callers will anyway do those
> +		 * checks extensively. However, in an assert-enabled build, we perform
> +		 * all the checks here and raise an error if failed.
> +		 */

Why?

> +		/* Count what is wanted, not the whole page. */
> +		if ((data + nbytes) <= (page + XLOG_BLCKSZ))
> +		{
> +			/* All the bytes are in one page. */
> +			nbatch += nbytes;
> +			ntotal += nbytes;
> +			nbytes = 0;
> +		}
> +		else
> +		{
> +			Size		navailable;
> +
> +			/*
> +			 * All the bytes are not in one page. Deduce available bytes on
> +			 * the current page, count them and continue to look for remaining
> +			 * bytes.
> +			 */
s/deducate/deduct/? Perhaps better subtract?

Greetings,

Andres Freund

---

From: Bharath Rupireddy <[email-masked]>
Date: Sat, 4 Nov 2023 20:55:00 +0530

> moving to atomics, we won't need the delicate comment in

I'm attaching the v15 patch set implementing the above ideas. Please
have a look.

---

From: Jeff Davis <[email-masked]>
Date: Sat, 04 Nov 2023 14:27:39 -0700

On Sat, 2023-11-04 at 20:55 +0530, Bharath Rupireddy wrote:
> +		XLogRecPtr	EndPtr =
> pg_atomic_read_u64(&XLogCtl->xlblocks[curridx]);
> +
> +		/*
> +		 * xlblocks value can be InvalidXLogRecPtr before
> the new WAL buffer
> +		 * page gets initialized in AdvanceXLInsertBuffer.
> In such a case
> +		 * re-read the xlblocks value under the lock to
> ensure the correct
> +		 * value is read.
> +		 */
> +		if (unlikely(XLogRecPtrIsInvalid(EndPtr)))
> +		{
> +			LWLockAcquire(WALBufMappingLock,
> LW_EXCLUSIVE);
> +			EndPtr = pg_atomic_read_u64(&XLogCtl-
> >xlblocks[curridx]);
> +			LWLockRelease(WALBufMappingLock);
> +		}
> +
> +		Assert(!XLogRecPtrIsInvalid(EndPtr));

Can that really happen? If the EndPtr is invalid, that means the page
is in the process of being cleared, so the contents of the page are
undefined at that time, right?

Regards,
	Jeff Davis

---

From: Bharath Rupireddy <[email-masked]>
Date: Sun, 5 Nov 2023 04:15:00 +0530

> undefined at that time, right?

My initial thoughts were this way - xlblocks is being read without
holding WALBufMappingLock in XLogWrite() and since we write
InvalidXLogRecPtr to xlblocks array elements temporarily before
MemSet-ting the page in AdvanceXLInsertBuffer(), the PANIC "xlog write
request %X/%X is past end of log %X/%X" might get hit if EndPtr read
from xlblocks is InvalidXLogRecPtr. FWIW, an Assert(false); within the
if (unlikely(XLogRecPtrIsInvalid(EndPtr))) block didn't hit in make
check-world.

It looks like my above understanding isn't correct because it can
never happen that the page that's being written to the WAL file gets
initialized in AdvanceXLInsertBuffer(). I'll remove this piece of code
in next version of the patch unless there are any other thoughts.

[1]
    /*
     * Within the loop, curridx is the cache block index of the page to
     * consider writing.  Begin at the buffer containing the next unwritten
     * page, or last partially written page.
     */
    curridx = XLogRecPtrToBufIdx(LogwrtResult.Write);

    while (LogwrtResult.Write < WriteRqst.Write)
    {
        /*
         * Make sure we're not ahead of the insert process.  This could happen
         * if we're passed a bogus WriteRqst.Write that is past the end of the
         * last page that's been initialized by AdvanceXLInsertBuffer.
         */
        XLogRecPtr  EndPtr = pg_atomic_read_u64(&XLogCtl->xlblocks[curridx]);

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 8 Nov 2023 13:10:34 +0530

> read the WAL before it was fully flushed.

Agreed and removed the check.

> interim we read bogus data, but that's a bit of a wasted effort.

In the new approach described upthread here
https://www.postgresql.org/message-id/c3455ab9da42e09ca9d059879b5c512b2d1f9681.camel%40j-davis.com,
there's no lock required for reading from WAL buffers. PSA patches for
more details.

> case?  We particularly shouldn't do so silently, seems that could hide all

I think it's possible to read a "just got initialized" page with the
new approach to read WAL buffer pages without WALBufMappingLock if the
page is read right after it is initialized and xlblocks is filled in
AdvanceXLInsertBuffer() but before actual WAL is written.

> > +              * all the checks here and raise an error if failed.

Minimal page header checks are performed to ensure we don't read the
page that just got initialized unlike what
XLogReaderValidatePageHeader(). Are you suggesting to remove page
header checks with XLogReaderValidatePageHeader() for assert-enabled
builds? Or are you suggesting to do page header checks with
XLogReaderValidatePageHeader() for production builds too?

PSA v16 patch set. Note that 0004 patch adds support for WAL read
stats (both from WAL file and WAL buffers) to walsenders and may not
necessarily the best approach to capture WAL read stats in light of
https://www.postgresql.org/message-id/CALj2ACU_f5_c8F%2BxyNR4HURjG%3DJziiz07wCpQc%3DAqAJUFh7%2B8w%40mail.gmail.com
which adds WAL read/write/fsync stats to pg_stat_io.

---

From: Andres Freund <[email-masked]>
Date: Thu, 9 Nov 2023 12:58:36 -0800

Hi,

> AdvanceXLInsertBuffer() but before actual WAL is written.

I think the code needs to make sure that *never* happens. That seems unrelated
to holding or not holding WALBufMappingLock. Even if the page header is
already valid, I don't think it's ok to just read/parse WAL data that's
concurrently being modified.

We can never allow WAL being read that's past
  XLogBytePosToRecPtr(XLogCtl->Insert->CurrBytePos)
as it does not exist.

And if the to-be-read LSN is between
XLogCtl->LogwrtResult->Write and XLogBytePosToRecPtr(Insert->CurrBytePos)
we need to call WaitXLogInsertionsToFinish() before copying the data.

Greetings,

Andres Freund

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 13 Nov 2023 19:02:07 +0530

>   XLogBytePosToRecPtr(XLogCtl->Insert->CurrBytePos)

Agreed. Erroring out in XLogReadFromBuffers() if passed in WAL is past
the CurrBytePos is an option. Another cleaner way is to just let the
caller decide what it needs to do (retry or error out) - fill an error
message in XLogReadFromBuffers() and return as-if nothing was read or
return a special negative error code like XLogDecodeNextRecord so that
the caller can deal with it.

Also, reading CurrBytePos with insertpos_lck spinlock can come in the
way of concurrent inserters. A possible way is to turn both
CurrBytePos and PrevBytePos 64-bit atomics so that
XLogReadFromBuffers() can read CurrBytePos without any lock atomically
and leave it to the caller to deal with non-existing WAL reads.

> we need to call WaitXLogInsertionsToFinish() before copying the data.

Agree to wait for all in-flight insertions to the pages we're about to
read to finish. But, reading XLogCtl->LogwrtRqst.Write requires either
XLogCtl->info_lck spinlock or WALWriteLock. Maybe turn
XLogCtl->LogwrtRqst.Write a 64-bit atomic and read it without any
lock, rely on
WaitXLogInsertionsToFinish()'s return value i.e. if
WaitXLogInsertionsToFinish() returns a value >= Insert->CurrBytePos,
then go read that page from WAL buffers.

Thoughts?

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 7 Dec 2023 15:59:00 +0530

> the caller can deal with it.

In the attached v17 patch, I've ensured that the XLogReadFromBuffers
returns when the caller requests a WAL that's past the current insert
position at the moment.

> then go read that page from WAL buffers.

In the attached v17 patch, I've ensured that the XLogReadFromBuffers
waits for all in-progress insertions to finish when the caller
requests WAL that's past the current write position and before the
current insert position.

I've also ensured that the XLogReadFromBuffers returns special return
codes for various scenarios (when asked to read in recovery, read on a
different TLI, read a non-existent WAL and so on.) instead of it
erroring out. This gives flexibility to the caller to decide what to
do.

---

From: Jeff Davis <[email-masked]>
Date: Thu, 07 Dec 2023 16:34:57 -0800

On Thu, 2023-12-07 at 15:59 +0530, Bharath Rupireddy wrote:
> In the attached v17 patch

0001 could impact performance could be impacted in a few ways:

   * There's one additional write barrier inside
     AdvanceXLInsertBuffer()
   * AdvanceXLInsertBuffer() already holds WALBufMappingLock, so
     the atomic access inside of it is somewhat redundant
   * On some platforms, the XLogCtlData structure size will change

The patch has been out for a while and nobody seems concerned about
those things, and they look fine to me, so I assume these are not real
problems. I just wanted to highlight them.

Also, the description and the comments seem off. The patch does two
things: (a) make it possible to read a page without a lock, which means
we need to mark with InvalidXLogRecPtr while it's being initialized;
and (b) use 64-bit atomics to make it safer (or at least more
readable).

(a) feels like the most important thing, and it's a hard requirement
for the rest of the work, right?

(b) seems like an implementation choice, and I agree with it on
readability grounds.

Also:

+  * But it means that when we do this
+  * unlocked read, we might see a value that appears to be ahead of
the
+  * page we're looking for. Don't PANIC on that, until we've verified
the
+  * value while holding the lock.

Is that still true even without a torn read?

The code for 0001 itself looks good. These are minor concerns and I am
inclined to commit something like it fairly soon.

Regards,
	Jeff Davis

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 20 Dec 2023 15:36:06 +0530

> inclined to commit something like it fairly soon.

Thanks. Attaching remaining patches as v18 patch-set after commits
c3a8e2a7cb16 and 766571be1659.

---

From: Jeff Davis <[email-masked]>
Date: Thu, 04 Jan 2024 17:50:05 -0800

Comments:

I still think the right thing for this patch is to call
XLogReadFromBuffers() directly from the callers who need it, and not
change WALRead(). I am open to changing this later, but for now that
makes sense to me so that we can clearly identify which callers benefit
and why. I have brought this up a few times before[1][2], so there must
be some reason that I don't understand -- can you explain it?

The XLogReadFromBuffersResult is never used. I can see how it might be
useful for testing or asserts, but it's not used even in the test
module. I don't think we should clutter the API with that kind of thing
-- let's just return the nread.

I also do not like the terminology "partial hit" to be used in this
way. Perhaps "short read" or something about hitting the end of
readable WAL would be better?

I like how the callers of WALRead() are being more precise about the
bytes they are requesting.

You've added several spinlock acquisitions to the loop. Two explicitly,
and one implicitly in WaitXLogInsertionsToFinish(). These may allow you
to read slightly further, but introduce performance risk. Was this
discussed?

The callers are not checking for XLREADBUGS_UNINITIALIZED_WAL, so it
seems like there's a risk of getting partially-written data? And it's
not clear to me the check of the wal page headers is the right one
anyway.

It seems like all of this would be simpler if you checked first how far
you can safely read data, and then just loop and read that far. I'm not
sure that it's worth it to try to mix the validity checks with the
reading of the data.

Regards,
	Jeff Davis

[1] https://www.postgresql.org/message-id/4132fe48f831ed6f73a9eb191af5fe475384969c.camel%40j-davis.com
[2]
https://www.postgresql.org/message-id/[email-masked]