Add the number of pinning backends to pg_buffercache's output
Hi,
The last week I twice had the need to see how many backends had some
buffers pinned. Once during development and once while analyzing a stuck
vacuum (waiting for a cleanup lock).
I'd like to add a column to pg_buffercache exposing that. The name I've
come up with is 'pinning_backends' to reflect the fact that it's not the
actual pincount due to the backend private arrays.
I'll add this patch to to 2014-06 CF.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
0001-Add-pinning_backends-column-to-the-pg_buffercache-ex.patchtext/x-patch; charset=us-asciiDownload
>From 32564c87e3323253ac8f77badc27628c2f4033c8 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Fri, 11 Apr 2014 22:01:36 +0200
Subject: [PATCH] Add pinning_backends column to the pg_buffercache extension.
---
contrib/pg_buffercache/Makefile | 2 +-
.../pg_buffercache/pg_buffercache--1.0--1.1.sql | 11 ++++++
contrib/pg_buffercache/pg_buffercache--1.0.sql | 20 -----------
contrib/pg_buffercache/pg_buffercache--1.1.sql | 21 +++++++++++
contrib/pg_buffercache/pg_buffercache.control | 2 +-
contrib/pg_buffercache/pg_buffercache_pages.c | 42 +++++++++++-----------
doc/src/sgml/pgbuffercache.sgml | 7 ++++
7 files changed, 61 insertions(+), 44 deletions(-)
create mode 100644 contrib/pg_buffercache/pg_buffercache--1.0--1.1.sql
delete mode 100644 contrib/pg_buffercache/pg_buffercache--1.0.sql
create mode 100644 contrib/pg_buffercache/pg_buffercache--1.1.sql
diff --git a/contrib/pg_buffercache/Makefile b/contrib/pg_buffercache/Makefile
index 323c0ac..5458a56 100644
--- a/contrib/pg_buffercache/Makefile
+++ b/contrib/pg_buffercache/Makefile
@@ -4,7 +4,7 @@ MODULE_big = pg_buffercache
OBJS = pg_buffercache_pages.o
EXTENSION = pg_buffercache
-DATA = pg_buffercache--1.0.sql pg_buffercache--unpackaged--1.0.sql
+DATA = pg_buffercache--1.1.sql pg_buffercache--1.0--1.1.sql pg_buffercache--unpackaged--1.0.sql
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/contrib/pg_buffercache/pg_buffercache--1.0--1.1.sql b/contrib/pg_buffercache/pg_buffercache--1.0--1.1.sql
new file mode 100644
index 0000000..4fd0ce4
--- /dev/null
+++ b/contrib/pg_buffercache/pg_buffercache--1.0--1.1.sql
@@ -0,0 +1,11 @@
+/* contrib/pg_buffercache/pg_buffercache--1.0--1.1.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION pg_buffercache" to load this file. \quit
+
+-- Upgrade view to 1.1. format
+CREATE OR REPLACE VIEW pg_buffercache AS
+ SELECT P.* FROM pg_buffercache_pages() AS P
+ (bufferid integer, relfilenode oid, reltablespace oid, reldatabase oid,
+ relforknumber int2, relblocknumber int8, isdirty bool, usagecount int2,
+ pincount int8);
diff --git a/contrib/pg_buffercache/pg_buffercache--1.0.sql b/contrib/pg_buffercache/pg_buffercache--1.0.sql
deleted file mode 100644
index 4ca4c44..0000000
--- a/contrib/pg_buffercache/pg_buffercache--1.0.sql
+++ /dev/null
@@ -1,20 +0,0 @@
-/* contrib/pg_buffercache/pg_buffercache--1.0.sql */
-
--- complain if script is sourced in psql, rather than via CREATE EXTENSION
-\echo Use "CREATE EXTENSION pg_buffercache" to load this file. \quit
-
--- Register the function.
-CREATE FUNCTION pg_buffercache_pages()
-RETURNS SETOF RECORD
-AS 'MODULE_PATHNAME', 'pg_buffercache_pages'
-LANGUAGE C;
-
--- Create a view for convenient access.
-CREATE VIEW pg_buffercache AS
- SELECT P.* FROM pg_buffercache_pages() AS P
- (bufferid integer, relfilenode oid, reltablespace oid, reldatabase oid,
- relforknumber int2, relblocknumber int8, isdirty bool, usagecount int2);
-
--- 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;
diff --git a/contrib/pg_buffercache/pg_buffercache--1.1.sql b/contrib/pg_buffercache/pg_buffercache--1.1.sql
new file mode 100644
index 0000000..c63b8af
--- /dev/null
+++ b/contrib/pg_buffercache/pg_buffercache--1.1.sql
@@ -0,0 +1,21 @@
+/* contrib/pg_buffercache/pg_buffercache--1.1.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION pg_buffercache" to load this file. \quit
+
+-- Register the function.
+CREATE FUNCTION pg_buffercache_pages()
+RETURNS SETOF RECORD
+AS 'MODULE_PATHNAME', 'pg_buffercache_pages'
+LANGUAGE C;
+
+-- Create a view for convenient access.
+CREATE VIEW pg_buffercache AS
+ SELECT P.* FROM pg_buffercache_pages() AS P
+ (bufferid integer, relfilenode oid, reltablespace oid, reldatabase oid,
+ relforknumber int2, relblocknumber int8, isdirty bool, usagecount int2,
+ pincount int8);
+
+-- 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;
diff --git a/contrib/pg_buffercache/pg_buffercache.control b/contrib/pg_buffercache/pg_buffercache.control
index 709513c..5494e2f 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.0'
+default_version = '1.1'
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 1e2d192..734a484 100644
--- a/contrib/pg_buffercache/pg_buffercache_pages.c
+++ b/contrib/pg_buffercache/pg_buffercache_pages.c
@@ -15,7 +15,8 @@
#include "storage/bufmgr.h"
-#define NUM_BUFFERCACHE_PAGES_ELEM 8
+#define NUM_BUFFERCACHE_PAGES_MIN_ELEM 8
+#define NUM_BUFFERCACHE_PAGES_ELEM 9
PG_MODULE_MAGIC;
@@ -36,6 +37,7 @@ typedef struct
bool isvalid;
bool isdirty;
uint16 usagecount;
+ int32 pinning_backends;
} BufferCachePagesRec;
@@ -62,7 +64,6 @@ pg_buffercache_pages(PG_FUNCTION_ARGS)
Datum result;
MemoryContext oldcontext;
BufferCachePagesContext *fctx; /* User function context. */
- TupleDesc tupledesc;
HeapTuple tuple;
if (SRF_IS_FIRSTCALL())
@@ -78,26 +79,17 @@ pg_buffercache_pages(PG_FUNCTION_ARGS)
/* Create a user function context for cross-call persistence */
fctx = (BufferCachePagesContext *) palloc(sizeof(BufferCachePagesContext));
- /* Construct a tuple descriptor for the result rows. */
- tupledesc = CreateTemplateTupleDesc(NUM_BUFFERCACHE_PAGES_ELEM, false);
- TupleDescInitEntry(tupledesc, (AttrNumber) 1, "bufferid",
- INT4OID, -1, 0);
- TupleDescInitEntry(tupledesc, (AttrNumber) 2, "relfilenode",
- OIDOID, -1, 0);
- TupleDescInitEntry(tupledesc, (AttrNumber) 3, "reltablespace",
- OIDOID, -1, 0);
- TupleDescInitEntry(tupledesc, (AttrNumber) 4, "reldatabase",
- OIDOID, -1, 0);
- TupleDescInitEntry(tupledesc, (AttrNumber) 5, "relforknumber",
- INT2OID, -1, 0);
- TupleDescInitEntry(tupledesc, (AttrNumber) 6, "relblocknumber",
- INT8OID, -1, 0);
- TupleDescInitEntry(tupledesc, (AttrNumber) 7, "isdirty",
- BOOLOID, -1, 0);
- TupleDescInitEntry(tupledesc, (AttrNumber) 8, "usage_count",
- INT2OID, -1, 0);
-
- fctx->tupdesc = BlessTupleDesc(tupledesc);
+ if (get_call_result_type(fcinfo, NULL, &fctx->tupdesc) != TYPEFUNC_COMPOSITE)
+ elog(ERROR, "return type must be a row type");
+
+ /*
+ * To smoothly support upgrades from version 1.0 of this extension
+ * transparently handle the (non-)existance of the pinning_backends
+ * column.
+ */
+ if (fctx->tupdesc->natts < NUM_BUFFERCACHE_PAGES_MIN_ELEM ||
+ fctx->tupdesc->natts > NUM_BUFFERCACHE_PAGES_ELEM)
+ elog(ERROR, "incorrect number of output arguments");
/* Allocate NBuffers worth of BufferCachePagesRec records. */
fctx->record = (BufferCachePagesRec *) palloc(sizeof(BufferCachePagesRec) * NBuffers);
@@ -134,6 +126,7 @@ pg_buffercache_pages(PG_FUNCTION_ARGS)
fctx->record[i].forknum = bufHdr->tag.forkNum;
fctx->record[i].blocknum = bufHdr->tag.blockNum;
fctx->record[i].usagecount = bufHdr->usage_count;
+ fctx->record[i].pinning_backends = bufHdr->refcount;
if (bufHdr->flags & BM_DIRTY)
fctx->record[i].isdirty = true;
@@ -188,6 +181,8 @@ pg_buffercache_pages(PG_FUNCTION_ARGS)
nulls[5] = true;
nulls[6] = true;
nulls[7] = true;
+ /* might not be used, but the array is long enough */
+ nulls[8] = true;
}
else
{
@@ -205,6 +200,9 @@ pg_buffercache_pages(PG_FUNCTION_ARGS)
nulls[6] = false;
values[7] = Int16GetDatum(fctx->record[i].usagecount);
nulls[7] = false;
+ /* might not be used, but the array is long enough */
+ values[8] = Int32GetDatum(fctx->record[i].pinning_backends);
+ nulls[8] = false;
}
/* Build and return the tuple. */
diff --git a/doc/src/sgml/pgbuffercache.sgml b/doc/src/sgml/pgbuffercache.sgml
index 4eb02c0..f379be2 100644
--- a/doc/src/sgml/pgbuffercache.sgml
+++ b/doc/src/sgml/pgbuffercache.sgml
@@ -106,6 +106,13 @@
<entry>Clock-sweep access count</entry>
</row>
+ <row>
+ <entry><structfield>pinning_backends</structfield></entry>
+ <entry><type>integer</type></entry>
+ <entry></entry>
+ <entry>Number of backends pinning this buffer</entry>
+ </row>
+
</tbody>
</tgroup>
</table>
--
1.8.3.251.g1462b67
On Sat, Apr 12, 2014 at 9:25 PM, Andres Freund <andres@2ndquadrant.com> wrote:
Hi,
The last week I twice had the need to see how many backends had some
buffers pinned. Once during development and once while analyzing a stuck
vacuum (waiting for a cleanup lock).
I'd like to add a column to pg_buffercache exposing that. The name I've
come up with is 'pinning_backends' to reflect the fact that it's not the
actual pincount due to the backend private arrays.
This name sounds good to me.
+CREATE OR REPLACE VIEW pg_buffercache AS
+ SELECT P.* FROM pg_buffercache_pages() AS P
+ (bufferid integer, relfilenode oid, reltablespace oid, reldatabase oid,
+ relforknumber int2, relblocknumber int8, isdirty bool, usagecount int2,
+ pincount int8);pincount should be pinning_backends here?
This may be harmless but pinning_backends should be defined as int4
rather than int8
because BufferDesc->refcount is just defined as unsigned and it's
converted to Datum
by Int32GetDatum().
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
s/CREATE/ALTER
+\echo Use "CREATE EXTENSION pg_buffercache" to load this file. \quit
The message should be something like "ALTER EXTENSION pg_buffercache
UPDATE TO '1.1'".
+ /* might not be used, but the array is long enough */
+ values[8] = Int32GetDatum(fctx->record[i].pinning_backends);
+ nulls[8] = false;Why is the above source comment needed?
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2014-06-23 18:44:24 +0900, Fujii Masao wrote:
On Sat, Apr 12, 2014 at 9:25 PM, Andres Freund <andres@2ndquadrant.com> wrote:
Hi,
The last week I twice had the need to see how many backends had some
buffers pinned. Once during development and once while analyzing a stuck
vacuum (waiting for a cleanup lock).
I'd like to add a column to pg_buffercache exposing that. The name I've
come up with is 'pinning_backends' to reflect the fact that it's not the
actual pincount due to the backend private arrays.This name sounds good to me.
+CREATE OR REPLACE VIEW pg_buffercache AS + SELECT P.* FROM pg_buffercache_pages() AS P + (bufferid integer, relfilenode oid, reltablespace oid, reldatabase oid, + relforknumber int2, relblocknumber int8, isdirty bool, usagecount int2, + pincount int8);pincount should be pinning_backends here?
Yes. I'd changed my mind around a couple of times and apparently didn't
send the right version of the patch. Thanks.
This may be harmless but pinning_backends should be defined as int4
rather than int8
because BufferDesc->refcount is just defined as unsigned and it's
converted to Datum
by Int32GetDatum().
Well, in theory a uint32 can't generally be converted to a int32. That's
why I chose a int64 because it's guaranteed to have sufficient
range. It's fairly unlikely to have that many pins, but using a int64
seems cheap enough here.
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
s/CREATE/ALTER
+\echo Use "CREATE EXTENSION pg_buffercache" to load this file. \quit
Hm, right.
The message should be something like "ALTER EXTENSION pg_buffercache
UPDATE TO '1.1'".+ /* might not be used, but the array is long enough */ + values[8] = Int32GetDatum(fctx->record[i].pinning_backends); + nulls[8] = false;Why is the above source comment needed?
It tries to explain that while the caller doesn't necessarily look at
values[8] (if it's the old pg_proc entry) but we're guaranteed to have
allocated a long enough values/nulls array.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jun 23, 2014 at 6:51 PM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2014-06-23 18:44:24 +0900, Fujii Masao wrote:
On Sat, Apr 12, 2014 at 9:25 PM, Andres Freund <andres@2ndquadrant.com> wrote:
Hi,
The last week I twice had the need to see how many backends had some
buffers pinned. Once during development and once while analyzing a stuck
vacuum (waiting for a cleanup lock).
I'd like to add a column to pg_buffercache exposing that. The name I've
come up with is 'pinning_backends' to reflect the fact that it's not the
actual pincount due to the backend private arrays.This name sounds good to me.
+CREATE OR REPLACE VIEW pg_buffercache AS + SELECT P.* FROM pg_buffercache_pages() AS P + (bufferid integer, relfilenode oid, reltablespace oid, reldatabase oid, + relforknumber int2, relblocknumber int8, isdirty bool, usagecount int2, + pincount int8);pincount should be pinning_backends here?
Yes. I'd changed my mind around a couple of times and apparently didn't
send the right version of the patch. Thanks.This may be harmless but pinning_backends should be defined as int4
rather than int8
because BufferDesc->refcount is just defined as unsigned and it's
converted to Datum
by Int32GetDatum().Well, in theory a uint32 can't generally be converted to a int32. That's
why I chose a int64 because it's guaranteed to have sufficient
range. It's fairly unlikely to have that many pins, but using a int64
seems cheap enough here.
Yep, you're right.
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
s/CREATE/ALTER
+\echo Use "CREATE EXTENSION pg_buffercache" to load this file. \quit
Hm, right.
The message should be something like "ALTER EXTENSION pg_buffercache
UPDATE TO '1.1'".+ /* might not be used, but the array is long enough */ + values[8] = Int32GetDatum(fctx->record[i].pinning_backends); + nulls[8] = false;Why is the above source comment needed?
It tries to explain that while the caller doesn't necessarily look at
values[8] (if it's the old pg_proc entry) but we're guaranteed to have
allocated a long enough values/nulls array.
Understood.
I think you can commit this patch after fixing some minor things.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 23 June 2014 15:22, Andres Freund Wrote:
This may be harmless but pinning_backends should be defined as int4
rather than int8 because BufferDesc->refcount is just defined as
unsigned and it's converted to Datum by Int32GetDatum().Well, in theory a uint32 can't generally be converted to a int32.
That's why I chose a int64 because it's guaranteed to have sufficient
range. It's fairly unlikely to have that many pins, but using a int64
seems cheap enough here.
But since we have declared pinning_backends as int8, we should use Int64GetDatum to form the tuple datum.
Using Int32GetDatum will lead to issue specially incase of WIN32 platform or any other platform where int8
is not considered as USE_FLOAT8_BYVAL (or FLOAT8PASSBYVAL as 0). On such platform while initializing the tuple,
type by value will be marked as false but still value (not address) will be passed to datum, which will lead to crash.
I have done testing to verify this on win32 and able to observe the crash where as it works fine on Linux.
Also can we change the description of function "pg_buffercache_pages" to include pinning_backend also in the description.
Thanks and Regards,
Kumar Rajeev Rastogi
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2014-07-02 08:55:05 +0000, Rajeev rastogi wrote:
Also can we change the description of function "pg_buffercache_pages" to include pinning_backend also in the description.
I'm not sure what you mean here - I've adjusted the docs to include the
column? Or do you mean that it errorneously was named pincount as Fujii
noticed?
Thanks for the review,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers