Re: Review: DTrace probes (merged version) ver_03 - Mailing list pgsql-hackers

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