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

Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
> 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 looked at this patch a little bit.  In addition to the comments Alvaro
made, I have a couple more issues:

* The probes that pass buffer tag elements are already broken by the
pending "relation forks" patch: there is soon going to be another field
in buffer tags.  Perhaps it'd be feasible to pass the buffer tag as a 
single probe argument to make that a bit more future-proof?  I'm not
sure if that would complicate the use of the probe so much as to be
counterproductive.

* I find this to be truly bletcherous:

> !     /* 
> !      * Due to a bug in Mac OS X 10.5, using defined types (e.g. uintptr_t,
> !      * uint32_t, etc.) cause compilation error.  
> !      */
> !       
> !     probe transaction__start(unsigned int transactionId);
> !     probe transaction__commit(unsigned int transactionId);
> !     probe transaction__abort(unsigned int transactionId);

especially since some of the manual translations in the file are flat
out wrong (Oid is unsigned for instance).  Furthermore the comment is
wrong, at least according to my tests with XCode 3.1.  Typedefs seem to
work fine.  What doesn't work fine is #include "postgres.h", which would
be the ideal way to suck in TransactionId and other needed typedefs.
You can get dtrace to invoke the C preprocessor, but at least on OS X,
the D compiler spits up anyway on the contents of some of the system
header files that are pulled in by postgres.h.

What I suggest might be a reasonable compromise is to copy needed
typedefs directly into the probes.d file:
typedef unsigned int LocalTransactionId;
provider postgresql {
probe transaction__start(LocalTransactionId);

This at least makes it possible to declare the probes cleanly,
and it's fairly obvious what to fix if the principal definition of
LocalTransactionId ever changes.  I don't have Solaris to test on, but
on OS X this seems to behave the way we'd like: the typedef itself isn't
copied into the emitted probes.h file, but the emitted extern
declarations use it.
        regards, tom lane