Re: Broken stuff in new dtrace probes

I wrote:
> Seems that we need to have been quality-checking the dtrace patches
> a bit more carefully.

I went over all the existing dtrace probe calls and fixed what seemed
clearly bogus, but there was one issue that seems like a bit of a
judgement call. In ReadBuffer_common() we have

isExtend = (blockNum == P_NEW);

/* Substitute proper block number if caller asked for P_NEW */
if (isExtend)
blockNum = smgrnblocks(smgr, forkNum);

TRACE_POSTGRESQL_BUFFER_READ_START(forkNum, blockNum,
smgr->smgr_rnode.spcNode,
smgr->smgr_rnode.dbNode,
smgr->smgr_rnode.relNode,
isLocalBuf);

if (isLocalBuf)
... etc etc ...

What's bothering me about this is that in the isExtend case, the
potentially significant cost of the smgrnblocks() call is not going
to get charged as part of the buffer read time. An easy answer is
to move the TRACE call in front of that, which would mean that in
the isExtend case the trace would see blockNum == P_NEW rather than
the actual block number. You could argue it either way as to whether
that's good or bad, perhaps -- it would make it a tad harder to match
up read_start and read_done calls, but it would also make it clearer
which calls are for file extension and which are ordinary reads.

Furthermore, an isExtend call doesn't actually do a read(), so lumping
them together with regular reads doesn't seem like quite the right thing
for performance measurement purposes anyway. Maybe we actually ought to
have different probes for isExtend and regular cases.

Comments?

regards, tom lane