Hello everyone, patch was rebased.
Thank you Tomas for your reviewing this patch and for your valuable
comments.
From the very beginning we had the misunderstanding with the naming of
meethods.
> It'd be really useful if you could provide actual numbers, explain what
> metrics you compare and how. I'm sure it makes sense to you but it's
> utterly confusing for everyone else, and it also makes it impossible to
> reproduce the benchmark.
I test it as I wrote earlier, I run it a several times collecting TPS in
each series, and calculate average value.
> 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.
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.
--
Ivan Kartyshov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
diff --git a/contrib/pg_buffercache/Makefile b/contrib/pg_buffercache/Makefile
index 497dbeb..9668d84 100644
--- a/contrib/pg_buffercache/Makefile
+++ b/contrib/pg_buffercache/Makefile
@@ -4,8 +4,9 @@ MODULE_big = pg_buffercache
OBJS = pg_buffercache_pages.o $(WIN32RES)
EXTENSION = pg_buffercache
-DATA = pg_buffercache--1.2.sql pg_buffercache--1.1--1.2.sql \
- pg_buffercache--1.0--1.1.sql pg_buffercache--unpackaged--1.0.sql
+DATA = pg_buffercache--1.3.sql pg_buffercache--1.2--1.3.sql \
+ pg_buffercache--1.1--1.2.sql pg_buffercache--1.0--1.1.sql \
+ pg_buffercache--unpackaged--1.0.sql
PGFILEDESC = "pg_buffercache - monitoring of shared buffer cache in real-time"
ifdef USE_PGXS
diff --git a/contrib/pg_buffercache/pg_buffercache--1.2--1.3.sql b/contrib/pg_buffercache/pg_buffercache--1.2--1.3.sql
new file mode 100644
index 0000000..5935326
--- /dev/null
+++ 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;
diff --git a/contrib/pg_buffercache/pg_buffercache--1.2.sql b/contrib/pg_buffercache/pg_buffercache--1.3.sql
similarity index 88%
rename from contrib/pg_buffercache/pg_buffercache--1.2.sql
rename to contrib/pg_buffercache/pg_buffercache--1.3.sql
index 6ee5d84..6b3f69e 100644
--- a/contrib/pg_buffercache/pg_buffercache--1.2.sql
+++ b/contrib/pg_buffercache/pg_buffercache--1.3.sql
@@ -1,4 +1,4 @@
-/* contrib/pg_buffercache/pg_buffercache--1.2.sql */
+/* contrib/pg_buffercache/pg_buffercache--1.3.sql */
-- complain if script is sourced in psql, rather than via CREATE EXTENSION
\echo Use "CREATE EXTENSION pg_buffercache" to load this file. \quit
@@ -18,4 +18,4 @@ CREATE VIEW pg_buffercache AS
-- Don't want these to be available to public.
REVOKE ALL ON FUNCTION pg_buffercache_pages() FROM PUBLIC;
-REVOKE ALL ON pg_buffercache FROM PUBLIC;
+REVOKE ALL ON pg_buffercache FROM PUBLIC;
\ No newline at end of file
diff --git a/contrib/pg_buffercache/pg_buffercache.control b/contrib/pg_buffercache/pg_buffercache.control
index a4d664f..8c060ae 100644
--- a/contrib/pg_buffercache/pg_buffercache.control
+++ b/contrib/pg_buffercache/pg_buffercache.control
@@ -1,5 +1,5 @@
# pg_buffercache extension
comment = 'examine the shared buffer cache'
-default_version = '1.2'
+default_version = '1.3'
module_pathname = '$libdir/pg_buffercache'
relocatable = true
diff --git a/contrib/pg_buffercache/pg_buffercache_pages.c b/contrib/pg_buffercache/pg_buffercache_pages.c
index da13bde..b6d2efc 100644
--- a/contrib/pg_buffercache/pg_buffercache_pages.c
+++ b/contrib/pg_buffercache/pg_buffercache_pages.c
@@ -136,15 +136,6 @@ pg_buffercache_pages(PG_FUNCTION_ARGS)
MemoryContextSwitchTo(oldcontext);
/*
- * To get a consistent picture of the buffer state, we must lock all
- * partitions of the buffer map. Needless to say, this is horrible
- * for concurrency. Must grab locks in increasing order to avoid
- * possible deadlocks.
- */
- for (i = 0; i < NUM_BUFFER_PARTITIONS; i++)
- LWLockAcquire(BufMappingPartitionLockByIndex(i), LW_SHARED);
-
- /*
* Scan through all the buffers, saving the relevant fields in the
* fctx->record structure.
*/
@@ -154,6 +145,12 @@ pg_buffercache_pages(PG_FUNCTION_ARGS)
uint32 buf_state;
bufHdr = GetBufferDescriptor(i);
+ if (bufHdr->tag.forkNum == -1)
+ {
+ fctx->record[i].blocknum = InvalidBlockNumber;
+ continue;
+ }
+
/* Lock each buffer header before inspecting. */
buf_state = LockBufHdr(bufHdr);
@@ -179,16 +176,6 @@ pg_buffercache_pages(PG_FUNCTION_ARGS)
UnlockBufHdr(bufHdr, buf_state);
}
-
- /*
- * And release locks. We do this in reverse order for two reasons:
- * (1) Anyone else who needs more than one of the locks will be trying
- * to lock them in increasing order; we don't want to release the
- * other process until it can get all the locks it needs. (2) This
- * avoids O(N^2) behavior inside LWLockRelease.
- */
- for (i = NUM_BUFFER_PARTITIONS; --i >= 0;)
- LWLockRelease(BufMappingPartitionLockByIndex(i));
}
funcctx = SRF_PERCALL_SETUP();
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
