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.
TRACE_POSTGRESQL_CLOG_CHECKPOINT_START(false) # shutdown case
>> 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
> 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)
>> bufHdr = LocalBufferAlloc(smgr, blockNum, &found);
>> if (found)
>> TRACE_POSTGRESQL_BUFFER_HIT(true); /* local buffer */
>> TRACE_POSTGRESQL_BUFFER_MISS(true); /* ditto */
>> * 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)
>> TRACE_POSTGRESQL_BUFFER_HIT(false); /* not local */
>> 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.
>> 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
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).
>> Why are there no probes for the v3 protocol stuff? There should
>> be probes for Parse, Bind, Execute message processing too, for
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
In response to
pgsql-hackers by date
|Next:||From: Robert Lor||Date: 2008-07-28 23:54:14|
|Subject: Re: Review: DTrace probes (merged version) ver_03|
|Previous:||From: Tom Lane||Date: 2008-07-28 23:18:14|
|Subject: Re: WITH RECUSIVE patches 0723 |