Zdenek Kotala wrote:
Alvaro Herrera napsal(a):
Zdenek Kotala wrote:
I performed review and I prepared own patch which contains only
probes without any issue. I suggest commit this patch because the
rest of patch is independent and it can be committed next commit
fest after rework.
I found following issues:
I noticed that CLOG, Subtrans and Multixact probes are added during a
regular Checkpoint, but not during a shutdown flush. I think the probes
should count that too (probably with the same counter).
Yeah, good catch.
Ok. I think the names should be the same but pass a true/false argument
to differentiate the call just like how it's used in SimpleLruFlush.
e.g.
TRACE_POSTGRESQL_CLOG_CHECKPOINT_START(false) # shutdown case
TRACE_POSTGRESQL_CLOG_CHECKPOINT_START(true)
In the pgstat_report_activity probe, is it good to call the probe before
taking the fast path out?
If you mean to put it behind "if (!pgstat_track_activities ||
!beentry)" then I think that current placement is OK. You should be
possibility to track it without dependency on pgstat_track_activities
GUC variable. Keep in mind that DTrace is designed to monitor/trace
production system and ability to monitor something without any
configuration change is main idea.
This probe just prints out the statement, and it doesn't matter whether
or not track_activities is enabled.
In the BUFFER_READ_START probe, we do not include the smgrnblocks()
call, which could be significant since it includes a number of system
calls.
It is because the BUFFER_READ_START needs to report correct blockno.
My suggestion is to add probes to smgrblock function.
Yes, that's the main reason.
I think BUFFER_HIT and BUFFER_MISS should include the "isLocalBuf" flag.
I also wonder whether BUFFER_HIT should be called in the block above,
lines 220-238, where we check the "found" flag, i.e.
if (isLocalBuf)
{
ReadLocalBufferCount++;
bufHdr = LocalBufferAlloc(smgr, blockNum, &found);
if (found)
{
LocalBufferHitCount++;
TRACE_POSTGRESQL_BUFFER_HIT(true); /* local buffer */
}
else
{
TRACE_POSTGRESQL_BUFFER_MISS(true); /* ditto */
}
}
else
{
ReadBufferCount++;
/*
* lookup the buffer. IO_IN_PROGRESS is set if the requested
block is
* not currently in memory.
*/
bufHdr = BufferAlloc(smgr, blockNum, strategy, &found);
if (found)
{
BufferHitCount++;
TRACE_POSTGRESQL_BUFFER_HIT(false); /* not local */
}
else
{
TRACE_POSTGRESQL_BUFFER_MISS(false); /* ditto */
}
}
(note that this changes the semantics w.r.t. the isExtend flag).
Good point. The question about isExted is if we want to have exact
output include corner case or be to sync with ReadBufferCount counter.
I prefer your suggested placement.
Agree.
I understand the desire to have DEADLOCK_FOUND, but is there really a
point in having a DEADLOCK_NOTFOUND probe? Since this code runs every
time someone waits for a lock longer than a second, there would be a lot
of useless counts and nothing useful.
No idea. Robert any comment?
Yes, you're right. Will delete.
I find it bogus that we include query rewriting in
QUERY_PARSE_START/DONE. I think query rewriting should be a separate
probe.
agree
Will fix. I was also thinking about having the probes by modules but
wanted to limit the number of probes, but I'm fine having another one.
QUERY_PLAN_START is badly placed -- it should be after the check for
utility commands (alternatively there could be a QUERY_PLAN_DONE in the
fast way out for utility commands, but in that case a "is utility" flag
would be needed. I don't see that there's any point in tracing planning
of utility commands though).
agree
Ok
Why are there no probes for the v3 protocol stuff? There should
be probes for Parse, Bind, Execute message processing too, for
completeness.
Thanks for catching this.
Also, I wonder if these probes should be in the for(;;)
loop in PostgresMain() instead of sprinkled in the other routines.
I note that the probes in PortalRun and PortalRunMulti are schizophrenic
about whether they include utility functions or not.
As are as I can tell, the current probes in PortalRun/Multi don't
include utility functions. Should they be included?
I'll send the patch for the above changes tomorrow!
--
Robert Lor Sun Microsystems
Austin, USA http://sun.com/postgresql
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
