Re: DTrace probes broken in HEAD on Solaris?

Dne 17.03.09 19:49, Tom Lane napsal(a):
> 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.
>
> More specifically, it seems that DTrace is designed so that it evaluates
> all the arguments to a probe macro before it decides whether to actually
> take the trap or not.

yeah, it is valid for USDT. I guess main reason for this is
implementation is that DTrace does not have idea how compiler generates
code and where he can find arguments. Only what it knows is how to call
a probe and this call is "noped".

> This seems to me to be a pretty bad/surprising behavior; it's not the
> way that our Assert macros work, for example. There's a performance
> issue, which would probably only be brutally obvious if you had an
> expensive function in the arguments. (Before you say no one would do
> that, note the relpath() calls I was complaining about last week.)
> But we've been muttering about dropping probes into some extremely
> hot hot-spots, like spinlock acquisition, and even a few more
> instructions to copy local variables could make a difference there.

yeah, spinlock is problem, but on other side the probes are only in the
branch when spinlock is not free and sleep anyway. And LWLOCK_ACQUIRE
probe has minimal impact if you compare it with rest of LWLockAcquire
function.

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

I don't accept your argument here. Better if it fails every time not
only when you enable the probe. DTrace is designated to be safe on
production machine. It is not good to shut down postgresql server in
production...

> 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 like this idea to used if(foo_ENABLED), but maybe we should used it
only when args evaluation is more complicated?

I'm not sure if it is good idea to modify macros definition.

See how macro definition look like:

#define TRACE_POSTGRESQL_LWLOCK_ACQUIRE(arg0, arg1) \
__dtrace_postgresql___lwlock__acquire(arg0, arg1)
#define TRACE_POSTGRESQL_LWLOCK_ACQUIRE_ENABLED() \
__dtraceenabled_postgresql___lwlock__acquire()

Maybe add something like this at the end of probe.h:

#define TRACE_POSTGRESQL_LWLOCK_ACQUIRE_COND(arg0, arg1) \
if( TRACE_POSTGRESQL_LWLOCK_ACQUIRE_ENABLED() ) \
TRACE_POSTGRESQL_LWLOCK_ACQUIRE(arg0, arg1)

Zdenek