Skip site navigation (1) Skip section navigation (2)

Re: Display Pg buffer cache (WIP)

From: Mark Kirkwood <markir(at)coretech(dot)co(dot)nz>
To: Neil Conway <neilc(at)samurai(dot)com>
Cc: pgsql-patches(at)postgresql(dot)org
Subject: Re: Display Pg buffer cache (WIP)
Date: 2005-03-03 07:39:12
Message-ID: 4226BF20.2020008@coretech.co.nz (view raw or flat)
Thread:
Lists: pgsql-patches
Neil Conway wrote:
> I don't like accessing shared data structures without acquiring the 
> appropriate locks; even if it doesn't break anything, it seems like just 
> asking for trouble. In order to be able to inspect a buffer's tag in 
> Tom's new locking scheme (not yet in HEAD, but will be in 8.1), you need 
> only hold a per-buffer lock. That will mean acquiring and releasing a 
> spinlock for each buffer, which isn't _too_ bad...
> 
> That means the data reported by the function might still be 
> inconsistent; not sure how big a problem that is.
> 
> It might be nice for the patch to indicate whether the buffers are 
> dirty, and what their shared reference count is.
> 

Yeah - sounds good, will do.

>> +extern Datum dump_cache(PG_FUNCTION_ARGS);
> 
> 

Yep.

> This declaration belongs in a header file (such as 
> include/utils/builtins.h).
> 
>> +typedef struct {
>> +    int                buffer;
>> +    AttInMetadata    *attinmeta;
>> +    BufferDesc        *bufhdr;
>> +    RelFileNode        rnode;
>> +    char            *values[3];
>> +} dumpcache_fctx;
> 
> 
> This should be values[4], no?
> 

Arrg... surprised there wasn't crashes everywhere....

> This is trivial, but I think most type names use camel case 
> (NamesLikeThis).
> 
> Why does `rnode' need to be in the struct? You can also fetch the buffer 
> number from the buffer desc, so you needn't store another copy of it.
> 
I thought it made readability a bit better, but yeah, could take it out.

>> +        /* allocate the strings for tuple formation */
>> +        fctx->values[0] = (char *) palloc(NAMEDATALEN + 1);
>> +        fctx->values[1] = (char *) palloc(NAMEDATALEN + 1);
>> +        fctx->values[2] = (char *) palloc(NAMEDATALEN + 1);
>> +        fctx->values[3] = (char *) palloc(NAMEDATALEN + 1);
> 
> 
> Is there a reason for choosing NAMEDATALEN + 1 as the size of these 
> buffers? (There is no relation between NAMEDATALEN and the number of 
> characters an OID will consume when converted via sprintf("%u") )

Hmmm... good spot, originally I was returning TEXTOID and relation names
etc...and then it was "big enough" after converting to oid during
testing, so err, yes - I will change that (as well) !
> 
> The patch doesn't treat unassigned buffers specially (i.e. those buffers 
> whose tag contains of InvalidOids). Perhaps it should return NULL for 
> their OID fields, rather than InvalidOid (which will be 0)? (An 
> alternative would be to not include those rows in the result set, 
> although perhaps administrators might want this information.)
> 
I thought it might be handy to explicitly see unused (or belonging to
another db) buffers. Clearly joining to pg_class will project 'em out!

Finally, thanks for the excellent feedback (fast too)

regards

Mark


In response to

pgsql-patches by date

Next:From: Zouari FouratDate: 2005-03-03 08:15:20
Subject: is it a bug ?
Previous:From: Neil ConwayDate: 2005-03-03 07:15:25
Subject: Re: Display Pg buffer cache (WIP)

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group