Re: Review: DTrace probes (merged version)

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