Re: Review: DTrace probes (merged version) ver_03

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.

> In the pgstat_re****t_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.

> 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 re****t correct blockno. My 
suggestion is to add probes to smgrblock function.

> 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 i***tend flag).

Good point. The question about i***ted 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?

> I find it bogus that we include query rewriting in
QUERY_PARSE_START/DONE.  
> I think query rewriting should be a separate probe.

agree

> 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

> Why are there no probes for the v3 protocol stuff?  There should
> be probes for Parse, Bind, Execute message processing too, for
> completeness.  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 ****talRun and ****talRunMulti are schizophrenic
> about whether they include utility functions or not.

+1 It is good suggestion. I hope that Robert will put it on his probe todo
list. 
Same like probes for memory management.

	Zdenek

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@[EMAIL PROTECTED]
)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers