On 09/29/2016 02:45 AM, Ivan Kartyshov wrote:
> > Secondly, I see this bit added to the loop over buffers:
> >
> > if (bufHdr->tag.forkNum == -1)
> > {
> > fctx->record[i].blocknum = InvalidBlockNumber;
> > continue;
> > }
> >
> > and I have no idea why this is needed (when it was not needed before).
>
> This helps to skip not used bufferpages. It is valuable on big and not
> warmup shared memory.
That seems like an unrelated change. Checking for forkNum, instead of
e.g. BM_VALID seems a bit surprising, too.
> On 09/02/2016 12:01 PM, Robert Haas wrote:
>> I think we certainly want to lock the buffer header, because otherwise
>> we might get a torn read of the buffer tag, which doesn't seem good.
>> But it's not obvious to me that there's any point in taking the lock
>> on the buffer mapping partition; I'm thinking that doesn't really do
>> anything unless we lock them all, and we all seem to agree that's
>> going too far.
>
> Replace consistent method with semiconsistent (that lock buffer headers
> without partition lock). Made some additional fixes thanks to reviewers.
This patch also does:
+++ b/contrib/pg_buffercache/pg_buffercache--1.2--1.3.sql
@@ -0,0 +1,6 @@
+/* contrib/pg_buffercache/pg_buffercache--1.2--1.3.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION pg_buffercache(text) UPDATE TO '1.3'" to
load this file. \quit
+
+ALTER FUNCTION pg_buffercache_pages() PARALLEL SAFE;
Why? That seems unrelated to what's been discussed in this thread.
I have committed the straightforward part of this, removing the
partition-locking. I think we're done here for this commitfest, but feel
free to post new patches for that PARALLEL SAFE and the quick-check for
unused buffers, if you think it's worthwhile.
- Heikki