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

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