Re: ReadRecentBuffer() doesn't scale well

Hi,

On 2023-06-29 19:35:30 +1200, Thomas Munro wrote:
> I (re)discovered why I used the lock-then-pin approach. In the
> comments I mentioned InvalidBuffer(), but the main problem is in its
> caller GetVictimBuffer() which has various sanity checks about
> reference counts that can occasionally fail if you have code randomly
> pinning any old buffer.

You're right. Specifically non-valid buffers are the issue.

> New idea: use the standard PinBuffer() function, but add a mode that
> doesn't pin invalid buffers (with caveat that you can perhaps get a
> false negative due to unlocked read, but never a false positive; see
> commit message). Otherwise we'd have to duplicate all the same logic
> to use cmpxchg for ReadRecentBuffer(), or rethink the assumptions in
> that other code.

It might be worth using lock free code in more places before long, but I agree
with the solution here.

> As for the lack of usage bump in the back-branches, I think the
> options are: teach PinBuffer_Locked() to increment it optionally, or
> back-patch whatever we come up with for this.

Hm, or just leave it as is.

> For the root buffer optimisation, the obvious place for storage seems
> to be under rd_amcache. It was originally invented for the cached
> metapage (commit d2896a9ed14) but could accommodate a new struct
> holding whatever we want. Here is a patch to try that out.
> BTAMCacheData would also be a natural place to put future things
> including a copy of the root page itself, in later work on lock-free
> tricks.

I am wondering if we don't want something more generic than stashing this in
rd_amcache. Don't want to end up duplicating relevant code across the uses of
rd_amcache in every AM.

> @@ -663,38 +663,17 @@ ReadRecentBuffer(RelFileLocator rlocator, ForkNumber forkNum, BlockNumber blockN
> else
> {
> bufHdr = GetBufferDescriptor(recent_buffer - 1);
> - have_private_ref = GetPrivateRefCount(recent_buffer) > 0;
>
> - /*
> - * Do we already have this buffer pinned with a private reference? If
> - * so, it must be valid and it is safe to check the tag without
> - * locking. If not, we have to lock the header first and then check.
> - */
> - if (have_private_ref)
> - buf_state = pg_atomic_read_u32(&bufHdr->state);
> - else
> - buf_state = LockBufHdr(bufHdr);
> -
> - if ((buf_state & BM_VALID) && BufferTagsEqual(&tag, &bufHdr->tag))
> + /* Is it still valid and holding the right tag? */
> + if (PinBuffer(bufHdr, NULL, true))

I do wonder if we should have an unlocked pre-check for a) the buffer being
valid and b) BufferTagsEqual() matching. With such a pre-check the race for
increasing the usage count of the wrong buffer is quite small, without the
pre-check it doesn't seem that small anymore.

Greetings,

Andres Freund