Tuesday, July 29, 2008

Re: [HACKERS] Review: DTrace probes (merged version) ver_03

Updated patch attached.

This patch addresses all of Alvaro's feedback except the new probes for
v3 protocol which will be submitted later along with the rest of the probes.

It also incorporates Tom's feedback as explained inline below.

I hope this patch is good to go for this commit fest. Will take care of
the rest in the next fest.

Tom Lane wrote:
> 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.
>
>
>
Took out the buffer tag argument for now. Will figure out how to best
solve this after this relation forks patch is committed.
>
> 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.
>
>
Implemented this suggestion. There are some weirdness with the OS X
compiler causing some of the probe declarations not to compile (see
comments in probe.d). The compiler spits out some warnings because the
types don't show up in the function prototype in probes.h, but the
probes work okay. I think we can safely ignore the warnings.

--
Robert Lor Sun Microsystems
Austin, USA http://sun.com/postgresql

No comments: