Re: Display Pg buffer cache (WIP) - Mailing list 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