From: | Robert Lor <Robert(dot)Lor(at)Sun(dot)COM> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: DTrace probes broken in HEAD on Solaris? |
Date: | 2009-03-24 21:31:43 |
Message-ID: | 49C9513F.6050907@sun.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Lor | 2009-03-24 21:40:53 | Re: Broken stuff in new dtrace probes |
Previous Message | Tom Lane | 2009-03-24 21:16:04 | Re: Re: [COMMITTERS] pgsql: Implement "fastupdate" support for GIN indexes, in which we try |