Re: Add the number of pinning backends to pg_buffercache's output

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