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
---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster
