Re: DTrace probes broken in HEAD on Solaris?

Tom Lane wrote:
> Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM> writes:
>
>> Answer why it happens when probes are disabled is, that for user
>> application there are piece of code which prepare DTrace probes
>> arguments which will be passed into kernel DTrace modul. This code has
>> performance penalty which depends on number of arguments.
>
>
> The other thing I don't like is that this implementation exposes people
> to any bugs that may exist in the probe arguments, *even when they don't
> have any tracing turned on*. (Again, we had two different instances of
> that last week, so don't bother arguing that it doesn't happen.)
>

Yes, the above scenario can occur when compiling with DTrace (passing
--enable-dtrace to configure) even when the probes are not enabled. It
won't be an issue if you don't compile with DTrace though as the macros
are converted to no-ops. Hopefully, any bugs in the probe arguments
would be caught during testing ;-)

> Both of these considerations are strong arguments for not building
> production installations with --enable-dtrace, just as we don't
> encourage people to build for production with --enable-cassert.
>
> But of course part of the argument for dtrace support is that people
> would like to have such probing available in production installations.
>
> What I've found out about this is that for each probe macro, DTrace
> also defines a foo_ENABLED() macro that can be used like this:
>
> if (foo_ENABLED())
> foo(...);
>
> I think what we should do about these considerations is fix our macro
> definitions so that the if(ENABLED()) test is built into the macros.
> I'm not sure what this will require ... probably some post-processing
> of the probes.h file ... but if we don't do it we're going to keep
> getting bit.
>
I was contemplating on using the is-enabled test, but when checking the
arguments to all the probes, they were quite simple (except the
relpath() call which is now removed).

I think the is-enabled test will address the issues you encountered. I
see a few alternative to fixing this:

1) Only use if (foo_ENABLED()) test for probes with expensive function
call/computation in arguments. This will keep the code clean, and we can
document this in the "Definine New Probes" section in the online doc.

2) Add the if(foo_ENABLED()) test to all probes manually and make this a
requirement for all future probes. This makes the check explicit and
avoid confusion.

3) Post-process probes.h so if(foo_ENABLED()) test is added to every
probe. We're doing some post-processing now by pre-pending TRACE_ to the
macros with a sed script. Personally, I don't like doing complex
post-processing of output from another tool because the script can break
if for some reason DTrace's output is changed.

I prefer option 1 the most and 3 the least.

-Robert