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

---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster

Reply via email to