Re: Review: DTrace probes (merged version) ver_03

Tom Lane napsal(a):
> Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)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.

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