Re: ReadRecentBuffer() doesn't scale well

From: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: Peter Geoghegan <pg(at)bowt(dot)ie>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: ReadRecentBuffer() doesn't scale well
Date: 2023-06-29 07:35:30
Message-ID: CA+hUKGLMFtNqei9nfcJy2SQMLWyYuO9E8NLYrb=4Gs1HgkAS7Q@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

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.

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.

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.

Experimental patches attached.

Attachment Content-Type Size
0001-Improve-ReadRecentBuffer-scalability.patch text/x-patch 5.8 KB
0002-Use-ReadRecentBuffer-for-btree-root-page.patch text/x-patch 7.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Richard Guo 2023-06-29 07:39:38 Re: Assert !bms_overlap(joinrel->relids, required_outer)
Previous Message Heikki Linnakangas 2023-06-29 07:06:35 Re: Add more sanity checks around callers of changeDependencyFor()