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