Display Pg buffer cache (WIP)

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

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

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...

I am looking at this bit now.

I take the first part to mean that don't need to wrap
LWLockAcquire(BufMgrLock, LW_EXCLUSIVE) ... LWLockRelease(BufMgrLock)
around the inspection of the buffers (?)

This per buffer lock, are we talking about LockBuffer(buf,
BUFFER_LOCK_SHARE) ... releaseBuffer(buf) ?

If so, it looks like I need to do some stuff with ResourceOwners,
otherwise ReleaseBuffer will fail (or am I wandering up the wrong track
here?). I am using anoncvs from yesterday, so if Tom's new scheme is
*very* new I may be missing it.

Thanks

Mark

Mark Kirkwood wrote:

If so, it looks like I need to do some stuff with ResourceOwners,
otherwise ReleaseBuffer will fail (or am I wandering up the wrong track
here?). I am using anoncvs from yesterday, so if Tom's new scheme is
*very* new I may be missing it.

It's so new, in fact, it's not in CVS yet :) I believe the latest
revision of the patch is here:

http://archives.postgresql.org/pgsql-patches/2005-02/msg00115.php

The locking scheme is described here:

http://archives.postgresql.org/pgsql-hackers/2005-02/msg00391.php

Holding the per-buffer header spinlock should be enough to ensure the
tag doesn't change. To get a globally consistent snapshot of the state
of the bufmgr, I believe it should be sufficient to also share-lock the
BufMappingLock for the duration of the scan. The latter will prevent new
pages from being loaded in the buffer pool, so I'm not sure if it's
worth doing. If you do decide to hold the BufMappingLock, it might make
sense to:

1. allocate an array of NBuffers elements
2. acquire BufferMappingLock in share mode
3. sequentially scan through the buffer pool, copying data into the array
4. release the lock
5. on each subsequent call to the SRF, format and return an element of
the array

Which should reduce the time to lock is held. This will require
allocating NBuffers * size_of_stats memory (where size_of_stats will be
something like 16 bytes).

-Neil

Mark Kirkwood <markir@coretech.co.nz> writes:

I am using anoncvs from yesterday, so if Tom's new scheme is
*very* new I may be missing it.

It's not committed yet ;-)

regards, tom lane

Tom Lane wrote:

Mark Kirkwood <markir@coretech.co.nz> writes:

I am using anoncvs from yesterday, so if Tom's new scheme is
*very* new I may be missing it.

It's not committed yet ;-)

Yep - that's pretty new, apologies for slow grey matter... I have been
following the discussion about the new buffer design, but didn't think
to connect it to this context!

cheers

Mark

Neil Conway wrote:

If you do decide to hold the BufMappingLock, it might make
sense to:

1. allocate an array of NBuffers elements
2. acquire BufferMappingLock in share mode
3. sequentially scan through the buffer pool, copying data into the array
4. release the lock
5. on each subsequent call to the SRF, format and return an element of
the array

Which should reduce the time to lock is held. This will require
allocating NBuffers * size_of_stats memory (where size_of_stats will be
something like 16 bytes).

That is a better approach, so I've used it in this new iteration.

In addition to holding the BufMappingLock, each buffer header is (spin)
locked before examining it, hopefully this is correct - BTW, I like the
new buffer lock design.

I'm still using BuildTupleFromCStrings, so there is considerable use of
sprintf conversion and "temporary" char * stuff. I would like this to be
a bit cleaner, so any suggestions welcome.

regards

Mark

Mark Kirkwood <markir@coretech.co.nz> writes:

In addition to holding the BufMappingLock, each buffer header is (spin)
locked before examining it, hopefully this is correct - BTW, I like the
new buffer lock design.

It'd be possible to dispense with the per-buffer spinlocks so long as
you look only at the tag (and perhaps the TAG_VALID flag bit). The
tags can't be changing while you hold the BufMappingLock. I'm dubious
that there's any point in recording information as transient as the
refcounts and dirtybits, and reducing the amount of time you hold the
lock would be a Good Thing.

Also, you're still not dealing with the case of a not-valid buffer in a
reasonable way.

regards, tom lane

Tom Lane wrote:

It'd be possible to dispense with the per-buffer spinlocks so long as
you look only at the tag (and perhaps the TAG_VALID flag bit). The
tags can't be changing while you hold the BufMappingLock.

That's what I had thought at first, but this comment in buf_internals.h
dissuaded me: "buf_hdr_lock must be held to examine or change the tag,
flags, usage_count, refcount, or wait_backend_id fields." The comment
already notes this isn't true if you've got the buffer pinned; it would
be worth adding another exception for holding the BufMappingLock, IMHO.

I'm dubious that there's any point in recording information as
transient as the refcounts and dirtybits

I think it's worth recording dirty bits -- it provides an indication of
the effectiveness of the bgwriter, for example. Reference counts could
be done away with, although I doubt it would have a significant effect
on the time spent holding the lock.

-Neil

Only two things to add: you forgot to add `cachedump.o' to the list of
OBJS in the utils/adt Makefile.

Mark Kirkwood wrote:

+typedef struct
+{
+	uint32		bufferid;
+	Oid			relfilenode;
+	Oid			reltablespace;
+	Oid			reldatabase;
+	bool		isdirty;
+	uint32		refcount;
+	BlockNumber	blocknum;
+
+} CacheDumpRec;

You should probably make `isdirty' the last member of this struct, so as
to reduce alignment/padding requirements (this won't actually save any
space right now, but it might save some space if more members are added
to the struct in the future).

-Neil

Neil Conway wrote:

Tom Lane wrote:

It'd be possible to dispense with the per-buffer spinlocks so long as
you look only at the tag (and perhaps the TAG_VALID flag bit). The
tags can't be changing while you hold the BufMappingLock.

That's what I had thought at first, but this comment in buf_internals.h
dissuaded me: "buf_hdr_lock must be held to examine or change the tag,
flags, usage_count, refcount, or wait_backend_id fields." The comment
already notes this isn't true if you've got the buffer pinned; it would
be worth adding another exception for holding the BufMappingLock, IMHO.

I'm dubious that there's any point in recording information as
transient as the refcounts and dirtybits

I think it's worth recording dirty bits -- it provides an indication of
the effectiveness of the bgwriter, for example. Reference counts could
be done away with, although I doubt it would have a significant effect
on the time spent holding the lock.

Let's suppose refcount is eliminated. I will then be examining the tag,
flags and buf_id elements of the buffer. Holding the BufMappingLock
prevents the tag changing, but what about the flags?

In addition Tom pointed out that I am not examining the BM_TAG_VALID or
BM_VALID flag bits (I am only checking if tag.blockNum equals
InvalidBlockNumber). My initial thought is to handle !BM_TAG_VALID or
!BM_VALID similarly to InvalidBlockNumber i.e all non buf_id fields set
to NULL.

Mark

The latest iteration.

I have added documentation and updated the expected output so that the
regression tests pass.

In addition, after looking at the various system view names, I decided
that 'pg_cache_dump' does not fit in nicely - so chose an more Pg
suitable name of 'pg_buffercache'. Some renaming of the backend
functions happened too. Finally, since I was saving blocknum, it went
into the view as well.

Hopefully I am dealing with invalid buffer tags sensibly now. The
per-buffer spin lock is still being held - altho it is obviously trivial
to remove if not actually required.

regards

Mark

P.s : remembered to use diff -c

Mark Kirkwood wrote:

Mark Kirkwood wrote:

+ 		tupledesc = CreateTemplateTupleDesc(NUM_BUFFERCACHE_PAGES_ELEM, false);
+ 		TupleDescInitEntry(tupledesc, (AttrNumber) 1, "bufferid",
+ 									INT4OID, -1, 0);
+ 		TupleDescInitEntry(tupledesc, (AttrNumber) 2, "relfilenode",
+ 									OIDOID, -1, 0);
+ 		TupleDescInitEntry(tupledesc, (AttrNumber) 3, "reltablespace",
+ 									OIDOID, -1, 0);
+ 		TupleDescInitEntry(tupledesc, (AttrNumber) 4, "reldatabase",
+ 									OIDOID, -1, 0);
+ 		TupleDescInitEntry(tupledesc, (AttrNumber) 5, "relblockbumber",
+ 									NUMERICOID, -1, 0);

I think this should be an int4, not numeric.

Otherwise, looks good to me. Barring any objections, I'll apply this
with a few stylistic tweaks and the numeric -> int4 change tomorrow.

-Neil

Neil Conway <neilc@samurai.com> writes:

Mark Kirkwood wrote:

+ 		TupleDescInitEntry(tupledesc, (AttrNumber) 5, "relblockbumber",
+ 									NUMERICOID, -1, 0);

I think this should be an int4, not numeric.

needs spell check too ;-)

More generally, BlockNumber is unsigned and so int4 is not at all an
accurate conversion. Perhaps OID would be a good choice even though
it's horribly wrong on one level.

Otherwise, looks good to me. Barring any objections, I'll apply this
with a few stylistic tweaks and the numeric -> int4 change tomorrow.

I would rather see this as a contrib module. There has been *zero*
consensus that we need this in core, nor any discussion about whether
it might be a security hole.

regards, tom lane

Tom Lane wrote:

Perhaps OID would be a good choice even though it's horribly wrong on
one level.

Either that or add unsigned numeric types to PG :-)

I would rather see this as a contrib module.

I don't really have an opinion either way. Does anyone else have
thoughts on this?

There has been *zero* consensus that we need this in core, nor any
discussion about whether it might be a security hole.

ISTM if it can only be invoked by the superuser, it should be safe enough.

-Neil

Neil Conway wrote:

Mark Kirkwood wrote:

+         TupleDescInitEntry(tupledesc, (AttrNumber) 5, "relblockbumber",
+                                     NUMERICOID, -1, 0); 

I think this should be an int4, not numeric.

I was looking for an UINT4OID :-), but numeric seemed the best
compromise (safer than overflowing int4). I guess I could add a type
'blocknumber' that is actually the same as 'oid', but it seems liks a
lot of effort for one column.

cheers

Mark

Tom Lane wrote:

I would rather see this as a contrib module. There has been *zero*
consensus that we need this in core, nor any discussion about whether
it might be a security hole.

Hmmm, I guess my motivation for thinking it might be useful in core was
that it was similar to 'pg_locks' and 'pg_stats', i.e. exposing some
internals in a convenient queryable manner (useful for problem solving).

On the security front, I should have added a 'REVOKE ALL ...FROM PUBLIC'
on the view to at least control access (fortunately easy to add).

cheers

Mark

Mark Kirkwood <markir@coretech.co.nz> writes:

Tom Lane wrote:

I would rather see this as a contrib module. There has been *zero*
consensus that we need this in core, nor any discussion about whether
it might be a security hole.

Hmmm, I guess my motivation for thinking it might be useful in core was
that it was similar to 'pg_locks' and 'pg_stats', i.e. exposing some
internals in a convenient queryable manner (useful for problem solving).

One reason for making it contrib is that I don't think you've got it
entirely right yet, and there will be several iterations before it
settles down. In a contrib module that is no problem, in core it's a
forced initdb each time.

regards, tom lane

Tom Lane wrote:

One reason for making it contrib is that I don't think you've got it
entirely right yet, and there will be several iterations before it
settles down. In a contrib module that is no problem, in core it's a
forced initdb each time.

Yeah - certainly less intrusive as a contrib if amendments are required!

Barring a huge groundswell of support for it in core :-) I will resubmit
a patch for it as a contrib module.

cheers

Mark