Mark Kirkwood wrote:
> does not worry too much about a consistent view of the buffer
> contents (as I didn't want it to block all other activity!).
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.
> +extern Datum dump_cache(PG_FUNCTION_ARGS);
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?
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.
> + /* 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") )
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.)
-Neil
