Re: 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

On Sat, Apr 12, 2014 at 9:25 PM, Andres Freund <andres(at)2ndquadrant(dot)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

On 2014-06-23 18:44:24 +0900, Fujii Masao wrote:
> On Sat, Apr 12, 2014 at 9:25 PM, Andres Freund <andres(at)2ndquadrant(dot)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

On Mon, Jun 23, 2014 at 6:51 PM, Andres Freund <andres(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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

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

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