Re: Review: DTrace probes (merged version) ver_03

Tom Lane napsal(a):
> Zdenek Kotala <Zdenek.Kotala@[EMAIL PROTECTED]
> 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.

It is difficult to say. If you pass only pointer to a structure you can
mine all 
data from them but it is little bit complicated. It should be possible to
use 
typedef and cast pointer to correct structure. However main problem is
that you 
cannot use easily any indirect data in predicates. It needs extra magic.
By my 
opinion best approach is use mix of approaches. Im****tant data should be
pass 
directly as a argument and rest as a pointer to data structure.

> * I find this to be truly bletcherous:

<snip>

 > What doesn't work fine is #include "postgres.h", which would
 > be the ideal way to suck in TransactionId and other needed typedefs.

Whats about #include "c.h"? Does it work or does it have same issue?

> 
> 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.

It works on Solaris as well. However, I think that better solution is to
put 
this typedef to the separate header file. DTrace script allows to use
typedef or 
include header with typedefs. It would be better to have header file which
could 
be used for probe.d and for any other DTrace script.


		Zdenek


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