Re: Review: DTrace probes (merged version) ver_03

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