Skip site navigation (1) Skip section navigation (2)

Re: Review: DTrace probes (merged version)

From: Robert Lor <Robert(dot)Lor(at)Sun(dot)COM>
To: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
Cc: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Robert Treat <xzilla(at)users(dot)sourceforge(dot)net>, jesus(at)omniti(dot)com
Subject: Re: Review: DTrace probes (merged version)
Date: 2008-07-21 19:47:24
Message-ID: 4884E7CC.60208@sun.com (view raw or flat)
Thread:
Lists: pgsql-hackers
Apologies for the delayed response - vacation, travel, etc got in the way!


Zdenek Kotala wrote:
> I performed review of merged patch from Robert Treat. At first point 
> the patch does not work (SunOS 5.11 snv_86 sun4u sparc 
> SUNW,Sun-Fire-V240)

The attached patch fixed the regression test errors.
>
>
> However I went through a code and I have following comments:
>
> 1) Naming convention:
>
>  - Some probes uses "*end", some "*done". It would be good to select 
> one name.
Noted. Will use <name>-done per the convention. This change will be 
included in an updated patch later since I think there are a number of 
other changes that need to be made.
>  - I prefer to use clog instead of slru in probes name. clog is widely 
> known.
I think slru- is okay per your subsequent emails.
>  - It seems to me better to have checkpoint-clog..., 
> checkpoint-subtrans instead of clog-checkpoint.
Yes, I was thinking about this too, but the current names are more 
consistent with the others. For example:

buffer-checkpoint, buffer-*
xlog-checkpoint, xlog-*
>  - buffer-flush was originally dirty-buffer-write-start. I prefer 
> Robert Lor's naming.
Actually, I made this change so the name is consistent with the other 
buffer-* probes.
>
> 2) storage read write probes
>
> smgr-read*, smgr-writes probes are in md.c. I personally think it make 
> more sense to put them into smgr.c. Only advantage to have it in md.c 
> is that actual amount of bytes is possible to monitor.
The current probes return number of bytes, that's why they are in md.c
>
> 3) query rewrite probe
>
> There are several probes for query measurement but query rewrite phase 
> is missing. See analyze_and_rewrite or pg_parse_and_rewrite
I believe the rewrite time is accounted for in the query-plan probe. 
Need to double check on this.
>
> 4) autovacuum_start
>
> Autovacuum_start probe is alone. I propose following probes for 
> completeness:
>
> proc-autovacuum-start
> proc-autovacuum-stop
> proc-bgwriter-start
> proc-bgwriter-stop
> proc-backend-start
> proc-backend-stop
> proc-master-start
> proc-master-stop
Saw a number of emails on this. Will get back on this.
>
> 5) Need explain of usage:
>
> I have some doubts about following probes. Could you please explain 
> usage of them? example dtrace script is welcome
>
>  - all exec-* probes
>  - mark-dirty, local-mark-dirty
>
Theo/Robert, do you guys have any sample scripts on the probes you added.
>
> 6) several comments about placement:
>
> I published patch on http://reviewdemo.postgresql.org/r/25/. I added 
> several comments there.
>
> 7) SLRU/CLOG
>
> SLRU probes could be return more info. For example if page was in 
> buffer or if physical write is not necessary and so on.
Yes, more info could be returned if we can identify specific use cases 
that the new data will enable.

-- 
Robert Lor           Sun Microsystems
Austin, USA          http://sun.com/postgresql


Attachment: 8.4_merged_probes_v2.patch
Description: text/plain (45.1 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Josh BerkusDate: 2008-07-21 19:53:05
Subject: Re: Do we really want to migrate plproxy and citext into PG core distribution?
Previous:From: Tom LaneDate: 2008-07-21 19:43:00
Subject: Do we really want to migrate plproxy and citext into PG core distribution?

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group