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)

     truncate             ... ok
     alter_table          ... FAILED (test process exited with exit code 2)
     sequence             ... ok
     polymorphism         ... ok
     rowtypes             ... ok
     returning            ... ok
     largeobject          ... FAILED (test process exited with exit code 2)
     xml                  ... ok
test stats                ... FAILED (test process exited with exit code 2)
test tablespace           ... FAILED (test process exited with exit code 2)


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.
 - I prefer to use clog instead of slru in probes name. clog is widely known.
- It seems to me better to have checkpoint-clog..., checkpoint-subtrans instead of clog-checkpoint. - buffer-flush was originally dirty-buffer-write-start. I prefer Robert Lor's naming.

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.

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

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

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


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.


        That's all for this moment

                        Zdenek          
--
Zdenek Kotala              Sun Microsystems
Prague, Czech Republic     http://sun.com/postgresql


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

Reply via email to