pgsql: Further dtrace adjustments for the backend-IDs-in-relpath patch.

rhaas@postgresql.org (Robert Haas) writes:

Further dtrace adjustments for the backend-IDs-in-relpath patch.

Hrm, this doesn't look right at all. The documentation now advertises
that backendid is passed to a number of probes that the code isn't
actually doing that for. I agree with the changes proposed by the
docs changes, and would suggest that buffer-flush-start,
buffer-flush-done, buffer-write-dirty-start, and buffer-write-dirty-done
probes probably need to get the backendid too. But in any case the
code isn't matching the docs in HEAD.

regards, tom lane

On Sat, Aug 14, 2010 at 12:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

rhaas@postgresql.org (Robert Haas) writes:

Further dtrace adjustments for the backend-IDs-in-relpath patch.

Hrm, this doesn't look right at all.  The documentation now advertises
that backendid is passed to a number of probes that the code isn't
actually doing that for.  I agree with the changes proposed by the
docs changes, and would suggest that buffer-flush-start,
buffer-flush-done, buffer-write-dirty-start, and buffer-write-dirty-done
probes probably need to get the backendid too.  But in any case the
code isn't matching the docs in HEAD.

Aargh. I thought I had checked this pretty carefully before
committing that last patch. I still can't see what I'm missing; here
are all the probes.d and monitoring.sgml entries that have been
changed in the last 10 revs of the git tree, matched up one for one:

+	probe buffer__read__start(ForkNumber, BlockNumber, Oid, Oid, Oid, int, bool);
     <entry>buffer-read-start</entry>
     <entry>(ForkNumber, BlockNumber, Oid, Oid, Oid, int, bool)</entry>
+	probe buffer__read__done(ForkNumber, BlockNumber, Oid, Oid, Oid,
int, bool, bool);
     <entry>buffer-read-done</entry>
     <entry>(ForkNumber, BlockNumber, Oid, Oid, Oid, int, bool, bool)</entry>
+	probe smgr__md__read__start(ForkNumber, BlockNumber, Oid, Oid, Oid, int);
     <entry>smgr-md-read-start</entry>
     <entry>(ForkNumber, BlockNumber, Oid, Oid, Oid, int)</entry>
+	probe smgr__md__read__done(ForkNumber, BlockNumber, Oid, Oid, Oid,
int, int, int);
     <entry>smgr-md-read-done</entry>
     <entry>(ForkNumber, BlockNumber, Oid, Oid, Oid, int, int, int)</entry>
+	probe smgr__md__write__start(ForkNumber, BlockNumber, Oid, Oid, Oid, int);
     <entry>smgr-md-write-start</entry>
     <entry>(ForkNumber, BlockNumber, Oid, Oid, Oid, int)</entry>
+	probe smgr__md__write__done(ForkNumber, BlockNumber, Oid, Oid, Oid,
int, int, int);
     <entry>smgr-md-write-done</entry>
     <entry>(ForkNumber, BlockNumber, Oid, Oid, Oid, int, int, int)</entry>

buffer-flush-start and buffer-flush-done are documented as only
getting called for shared buffers, so there is no point in passing a
backend ID that will always be -1. buffer-write-dirty-start and
buffer-write-dirty-done are not documented as applying only to shared
buffers, but I believe it to be the case: they are called from
BufferAlloc, which appears to be shared-buffers-only code.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas <robertmhaas@gmail.com> writes:

On Sat, Aug 14, 2010 at 12:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hrm, this doesn't look right at all.

Aargh. I thought I had checked this pretty carefully before
committing that last patch.

No, sorry, my mistake: I assumed your first commit hadn't touched the
probes at all, which I now see wasn't true. It does appear to be
consistent now.

buffer-flush-start and buffer-flush-done are documented as only
getting called for shared buffers, so there is no point in passing a
backend ID that will always be -1. buffer-write-dirty-start and
buffer-write-dirty-done are not documented as applying only to shared
buffers, but I believe it to be the case: they are called from
BufferAlloc, which appears to be shared-buffers-only code.

I wonder though whether we should take the opportunity to generalize the
probe definitions so that they would work for local buffers. But maybe
nobody really cares.

regards, tom lane

On Sun, Aug 15, 2010 at 2:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Sat, Aug 14, 2010 at 12:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hrm, this doesn't look right at all.

Aargh.  I thought I had checked this pretty carefully before
committing that last patch.

No, sorry, my mistake: I assumed your first commit hadn't touched the
probes at all, which I now see wasn't true.  It does appear to be
consistent now.

OK, good. :-)

buffer-flush-start and buffer-flush-done are documented as only
getting called for shared buffers, so there is no point in passing a
backend ID that will always be -1.  buffer-write-dirty-start and
buffer-write-dirty-done are not documented as applying only to shared
buffers, but I believe it to be the case: they are called from
BufferAlloc, which appears to be shared-buffers-only code.

I wonder though whether we should take the opportunity to generalize the
probe definitions so that they would work for local buffers.  But maybe
nobody really cares.

Well, *I* don't care. But I also don't mind it if someone who *does*
care writes a patch. As you can tell, I don't normally enable dtrace
functionality in my builds. :-(

Or to put it another way, I see no particularly compelling reason why
we need to do this right now, but neither do I object to it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company