DTrace probe patch for OS X Leopard

Please find the patch attached per this thread
http://archives.postgresql.org/pgsql-hackers/2008-02/msg00912.php

Notes to committer.

1) Please remove src/include/pg_trace.h as it's no longer needed

2) Need help figuring out how to copy src/backend/util/probes.d from src
tree to
bld tree at build time. It works fine if compilation is done in the src
tree.

3) Note on src/backend/Makefile
The current rule below does not work. After expansion, utils/probes.d
needs
to come right after -s, but currently it shows up at the end after
all the .o files.

utils/probes.o: utils/probes.d $(SUBDIROBJS)
$(DTRACE) $(DTRACEFLAGS) -G -s $(call expand_subsys,$^) -o $@

The following works, but I think the correct way is to include
probes.d as a
dependency and have it show up after -s. I couldn't get it to work
this way with
my somewhat limited knowledge of makefiles.

utils/probes.o: $(SUBDIROBJS)
$(DTRACE) $(DTRACEFLAGS) -G -s $(srcdir)/utils/probes.d -o $@
$(call expand_subsys,$^)

Regards,
-Robert

Robert Lor wrote:

> 3) Note on src/backend/Makefile
> The current rule below does not work. After expansion, utils/probes.d
> needs
> to come right after -s, but currently it shows up at the end after all
> the .o files.
>
> utils/probes.o: utils/probes.d $(SUBDIROBJS)
> $(DTRACE) $(DTRACEFLAGS) -G -s $(call expand_subsys,$^) -o $@

Perhaps you need a $< there:

$(DTRACE) $(DTRACEFLAGS) -G -s $< $(call expand_subsys,$^) -o $@

However I think you would also need to $(filter-out) the $< from $^.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Perhaps you need a $< there:
>
> $(DTRACE) $(DTRACEFLAGS) -G -s $< $(call expand_subsys,$^) -o $@
>
> However I think you would also need to $(filter-out) the $< from $^.
>
>
Your suggestion with $(filter-out ...) works. Thanks!

Attached is the updated patch.

Regards,
-Robert

Robert Lor wrote:
> 3) Note on src/backend/Makefile
>    The current rule below does not work. After expansion, utils/probes.d
> needs
>    to come right after -s, but currently it shows up at the end after
> all the .o files.

I fixed that part. Note again that a buildfarm animal testing the dtrace
support could be helpful. :)

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Robert Lor wrote:
> Please find the patch attached per this thread
> http://archives.postgresql.org/pgsql-hackers/2008-02/msg00912.php

Why do we have two dtrace calls in the makefiles now? I understand you added
the "new" mechanism to support Mac OS X, but doesn't Solaris support that
mechanism as well, so the old one could be dropped?

Btw., probes_null.h is missing in your patch.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Robert Lor wrote:
> 2) Need help figuring out how to copy src/backend/util/probes.d from src
> tree to
> bld tree at build time. It works fine if compilation is done in the src
> tree.

I have reworked your build rules so they look more like the idioms that we
already use for other similar cases. This should fix the troubles you
describe and others.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut wrote:
> Robert Lor wrote:
>
>> Please find the patch attached per this thread
>> http://archives.postgresql.org/pgsql-hackers/2008-02/msg00912.php
>>
>
> Why do we have two dtrace calls in the makefiles now?
The build process for Mac OS X is different than that of Solaris. The
dtrace call in src/Makefile is to generate probes.h before any file is
compiled so it can be used in c.h to avoid "probes.h not found" error.
The dtrace call in src/backend/Makefile is only needed for Solaris.
> I understand you added
> the "new" mechanism to support Mac OS X, but doesn't Solaris support that
> mechanism as well, so the old one could be dropped?
>
Both are needed.
> Btw., probes_null.h is missing in your patch.
>
>
I included this file (separate from the patch) in the first email.

Regards,
-Robert

Peter Eisentraut wrote:
> I have reworked your build rules so they look more like the idioms that we
> already use for other similar cases. This should fix the troubles you
> describe and others.
>
There are a couple of problems with your updated patch:

1) utils/probes.h needs to be generated before any file is compiled
since it's used in c.h and a lot of files include c.h. That's why in my
previous patch, I had a rule to call "$(DTRACE) -h -s $< -o $@" in
src/Makefile, and I don't think it will work putting it in
src/backend/utils/Makefile. If utils/probes.h doesn't exist, you get
compilation errors right of the bat.

...
gmake -C port all
gmake[2]: Entering directory `/export/home/pgs/src/pgsql/src/port'
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Winline
-Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing
-I../../src/port -DFRONTEND -I../../src/include -c -o isinf.o isinf.c
-MMD -MP -MF .deps/isinf.Po
In file included from isinf.c:15:
../../src/include/c.h:60:26: utils/probes.h: No such file or directory
gmake[2]: *** [isinf.o] Error 1
gmake[2]: Leaving directory `/export/home/pgs/src/pgsql/src/port'
gmake[1]: *** [all] Error 2
gmake[1]: Leaving directory `/export/home/pgs/src/pgsql/src'
gmake: *** [all] Error 2

It there a simple way to link to src/backend/utils/probes.d from a build
tree and put the generated probes.h in src/include/utils instead of
generate probes.h in src/backend/utils and link to in from
src/include/utils?

2) c.h needs to have an ifdef like below. probes_null.h is attached, and
this file is static and is used in the case Dtrace is not enabled.

#ifdef ENABLE_DTRACE
#include "utils/probes.h"
#else
#include "utils/probes_null.h"
#endif

Regards,
-Robert

Peter,

Robert Lor wrote:
> Peter Eisentraut wrote:
>> I have reworked your build rules so they look more like the idioms
>> that we already use for other similar cases. This should fix the
>> troubles you describe and others.
>>
> There are a couple of problems with your updated patch:
Based on your patch, I made a few changes and everything works now.
Patch attached!

Thanks for your help!

Regards,
-Robert

Robert Lor wrote:
> dtrace call in src/Makefile is to generate probes.h before any file is
> compiled so it can be used in c.h to avoid "probes.h not found" error.
> The dtrace call in src/backend/Makefile is only needed for Solaris.

Is c.h the right place to include this? The probes are only in the backend
code, so perhaps postgres.h would be more appropriate. Or just include it in
the .c files that need it, of which there are only 3.

Peter Eisentraut wrote:
> Is c.h the right place to include this? The probes are only in the backend
> code, so perhaps postgres.h would be more appropriate. Or just include it in
> the .c files that need it, of which there are only 3.
>
I think putting probes.h in c.h is the right place. It's true that the
probes are only in the backend now and only in a few files, but in the
future I can foresee probes added to more files in the backend, the
procedural language code or any of the commands (initdb, psql, etc).

Regards,
-Robert

Robert Lor <Robert(dot)Lor(at)Sun(dot)COM> writes:
> Peter Eisentraut wrote:
>> Is c.h the right place to include this? The probes are only in the backend
>> code, so perhaps postgres.h would be more appropriate. Or just include it in
>> the .c files that need it, of which there are only 3.
>>
> I think putting probes.h in c.h is the right place. It's true that the
> probes are only in the backend now and only in a few files, but in the
> future I can foresee probes added to more files in the backend, the
> procedural language code or any of the commands (initdb, psql, etc).

I agree with Peter. There are a whole lot of include files that are
needed by way more than 3 .c files, and yet are not folded into
postgres.h. c.h is right out.

regards, tom lane

Robert Lor wrote:
> Please find the patch attached per this thread
> http://archives.postgresql.org/pgsql-hackers/2008-02/msg00912.php

Another thing that is concerning me about this new approach is the way the
probes are named. For example, we'd now have a call

POSTGRESQL_LWLOCK_ACQUIRE()

in the code. This does not say we are *tracing* lock aquisition, but it looks
like a macro that actually acquires a lock.

I understand that these probe names follow some global naming scheme. Is it
hard to change that? I'd feel more comfortable with, say,
(D)TRACE_POSTGRESQL_LWLOCK_ACQUIRE().

Comments?

Tom Lane wrote:
> I agree with Peter. There are a whole lot of include files that are
> needed by way more than 3 .c files, and yet are not folded into
> postgres.h. c.h is right out.
>
My concern is that when we start adding more probes (not just the
backend), we will have to add the following 5 lines in .c files that use
the Dtrace macros. This seems intrusive and messy to me instead of in a
centralized place like c.h. What are the disadvantages for keeping the
way it is now?

#ifdef ENABLE_DTRACE
#include "utils/probes.h"
#else
#include "utils/probes_null.h"
#endif

Regards,
-Robert

Robert Lor wrote:

> My concern is that when we start adding more probes (not just the
> backend), we will have to add the following 5 lines in .c files that use
> the Dtrace macros. This seems intrusive and messy to me instead of in a
> centralized place like c.h. What are the disadvantages for keeping the
> way it is now?
>
> #ifdef ENABLE_DTRACE
> #include "utils/probes.h"
> #else
> #include "utils/probes_null.h"
> #endif

Why can't this block be centralized in probes.h?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Peter Eisentraut wrote:
> Another thing that is concerning me about this new approach is the way the
> probes are named. For example, we'd now have a call
>
> POSTGRESQL_LWLOCK_ACQUIRE()
>
> in the code. This does not say we are *tracing* lock aquisition, but it looks
> like a macro that actually acquires a lock.
>
Definitely a valid concern.
> I understand that these probe names follow some global naming scheme. Is it
> hard to change that? I'd feel more comfortable with, say,
> (D)TRACE_POSTGRESQL_LWLOCK_ACQUIRE().
>
Because the macro is auto generated and follows certain naming
conventions, prepending TRACE_ will not work. If you did that, the probe
name will be called "postgresql-lwlock-aquire" and the provider will be
"trace" which is not what we want.

To avoid the confusion, how about just adding a simple comment like /*
DTrace probe or Trace point or something similar */ before all
occurrences of the macro calls?

Regards,
-Robert

Alvaro Herrera wrote:
> Robert Lor wrote:
>
>
>> My concern is that when we start adding more probes (not just the
>> backend), we will have to add the following 5 lines in .c files that use
>> the Dtrace macros. This seems intrusive and messy to me instead of in a
>> centralized place like c.h. What are the disadvantages for keeping the
>> way it is now?
>>
>> #ifdef ENABLE_DTRACE
>> #include "utils/probes.h"
>> #else
>> #include "utils/probes_null.h"
>> #endif
>>
>
> Why can't this block be centralized in probes.h?
>
probes.h is auto generated and it can certainly be massaged to include
the above logic, but I'd like to avoid doing that if possible.

The thinking initially was to make this tracing feature more like a
"framework" and make it as simple as possible to add new probes and as
un-intrusive as possible, that's why I thought and still think that
putting the includes in c.h makes sense, unless there are obvious
disadvantages I'm not aware of.

Regards,
-Robert

Robert Lor wrote:
> Alvaro Herrera wrote:
>>
>> Why can't this block be centralized in probes.h?
>>
> probes.h is auto generated and it can certainly be massaged to include
> the above logic, but I'd like to avoid doing that if possible.

Hmm, so let's have a third file that's not autogenerated, which is the
file we will use for #includes, and contains just that block.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Robert Lor wrote:
>> probes.h is auto generated and it can certainly be massaged to include
>> the above logic, but I'd like to avoid doing that if possible.

> Hmm, so let's have a third file that's not autogenerated, which is the
> file we will use for #includes, and contains just that block.

Or just two files. Call probes_null something else, have it be included
where needed, have it include the autogenerated file when appropriate.

regards, tom lane

Robert Lor <Robert(dot)Lor(at)Sun(dot)COM> writes:
> Peter Eisentraut wrote:
>> I understand that these probe names follow some global naming scheme. Is it
>> hard to change that? I'd feel more comfortable with, say,
>> (D)TRACE_POSTGRESQL_LWLOCK_ACQUIRE().
>>
> Because the macro is auto generated and follows certain naming
> conventions, prepending TRACE_ will not work. If you did that, the probe
> name will be called "postgresql-lwlock-aquire" and the provider will be
> "trace" which is not what we want.

Ugh. Is this tool really so badly designed that it thinks it has
ownership of the *entire* namespace within the target program?

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>
>> Hmm, so let's have a third file that's not autogenerated, which is the
>> file we will use for #includes, and contains just that block.
>>
>
> Or just two files. Call probes_null something else, have it be included
> where needed, have it include the autogenerated file when appropriate.
>
>

I really like this idea. So, we're now back to having pg_trace.h which
includes the autogenerated probes.h when Dtrace is enabled, else null
macros will be used.

Currently, pg_trace.h is included in c.h, and I feel strongly that it
should remains there because by design I'd like to
1) have the tracing feature be available both in the frontend and
backend without having to do anything extra, which also means that
probes.h needs to be generated before any compilation
2) centralize the include of this header just in case the
implementation needs to be changed for some reason (eg, if this file
needs to be splitted, etc)
3) reduce the number of changes to a minimal when adding new probes to
new .c files

I haven't heard any major disadvantages about keeping it in c.h, but if
you are still adamant about keeping it out of c.h, I'll will go along
with that.

Regards,
-Robert

Robert Lor <Robert(dot)Lor(at)Sun(dot)COM> writes:
> I haven't heard any major disadvantages about keeping it in c.h, but if
> you are still adamant about keeping it out of c.h, I'll will go along
> with that.

I was thinking that pg_trace.h involved some backend-only code, but
I'm not sure why I thought that :-(. Yeah, your plan to do it by
restructuring the contents of pg_trace.h sounds fine.

We still have what I consider a big problem with the names of the
macros. Perhaps that could be fixed by passing the auto-generated
file through a sed script to put a prefix on the macro names before
we start to use it?

regards, tom lane

Am Freitag, 29. Februar 2008 schrieb Robert Lor:
> My concern is that when we start adding more probes (not just the
> backend), we will have to add the following 5 lines in .c files that use
> the Dtrace macros.

I had already solved this in my intermediate patch I sent you by symlinking
probes_null.h to probes.h.

Am Freitag, 29. Februar 2008 schrieb Robert Lor:
> Currently, pg_trace.h is included in c.h, and I feel strongly that it
> should remains there because by design I'd like to
> 1) have the tracing feature be available both in the frontend and
> backend without having to do anything extra,

I think each component would have its own probes definition file.

> which also means that
> probes.h needs to be generated before any compilation

Well, you are going to have to do a lot more work on the makefiles if you want
to do it that way. Make works by defining dependencies between files, not by
hoping that people will execute the commands in the order you write them. If
you want every single file in the tree to depend on a rule, you will have to
do something different.

> 2) centralize the include of this header just in case the
> implementation needs to be changed for some reason (eg, if this file
> needs to be splitted, etc)

We have no evidence that anything like that will ever happen.

> 3) reduce the number of changes to a minimal when adding new probes to
> new .c files

These arguments seem irrelevant in my mind. When you add new function calls,
you will usually have to add new header files as well. It's the normal way
to do things.

> I haven't heard any major disadvantages about keeping it in c.h, but if
> you are still adamant about keeping it out of c.h, I'll will go along
> with that.

Including only what you need is a principle. It keeps the namespace clean, it
speads up compilation time, it makes the build system simpler and more
efficient. Otherwise we'd only need one header file for everything.

Tom Lane wrote:
> We still have what I consider a big problem with the names of the
> macros. Perhaps that could be fixed by passing the auto-generated
> file through a sed script to put a prefix on the macro names before
> we start to use it?
>

Post processing the auto generated header may work, but I think it could
be unnecessarily complicated and error proned. First, the formats of the
header between Mac OS X and Solaris are different, and I'm pretty sure
it'll be different for FreeBSD when it comes out with Dtrace support.
Second, if there are changes in the content of the header in the
future, the sed script may break. I can investigate this approach
further, but I personally prefer a solution this is less error proned.

And don't think adding a simple comment before the macro call is
sufficient? This can be documented so everyone knows the convention.

Regards,
-Robert

Peter Eisentraut wrote:
> I had already solved this in my intermediate patch I sent you by symlinking
> probes_null.h to probes.h.
>
>

Now I see why you created the symlink. But I thinkt the suggestion by
Tom/Avaro to include probes.h and the content of probes_null.h in a
separate header (pg_trace.h) is a good one.

Regards,
-Robert

Robert Lor <Robert(dot)Lor(at)Sun(dot)COM> writes:
> And don't think adding a simple comment before the macro call is
> sufficient? This can be documented so everyone knows the convention.

It's stupid. The need for a comment is proof that the macro is badly
named. I don't intend to hold still for letting poorly-designed tools
dictate our coding conventions.

regards, tom lane

Robert Lor wrote:

> Post processing the auto generated header may work, but I think it could
> be unnecessarily complicated and error proned.

Would it work to name the traces "trace_transaction__start" etc instead?
AFAICS that would cause the macros to be named

POSTGRESQL_TRACE_TRANSACTION_START()

which is not ideal but at least it's a bit more obvious what it's all
about.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Peter Eisentraut wrote:
> I think each component would have its own probes definition file.
>

A while back when we met in Toronto, the consensus was to only have one
provider called "postgresql" and all probes whether they be from the
backend or frontend will be grouped together in this one provider.

> Well, you are going to have to do a lot more work on the makefiles if you want
> to do it that way. Make works by defining dependencies between files, not by
> hoping that people will execute the commands in the order you write them. If
> you want every single file in the tree to depend on a rule, you will have to
> do something different.
>
>
Actually, I was able to get it to work without doing much based on your
patch. Please comment on this updated patch I submitted yesterday
http://archives.postgresql.org/pgsql-patches/2008-02/msg00173.php

> Including only what you need is a principle. It keeps the namespace clean, it
> speads up compilation time, it makes the build system simpler and more
> efficient. Otherwise we'd only need one header file for everything.
>

Okay, will move the header into individual .c files.

Regards,
-Robert

Alvaro Herrera wrote:
> Would it work to name the traces "trace_transaction__start" etc instead?
> AFAICS that would cause the macros to be named
>
> POSTGRESQL_TRACE_TRANSACTION_START()
>
Correct, and that would work. With this approach, all the probe names
will start with trace-, and this particular one will be called
trace-transaction-start and can be used this way.

postgresql*:::trace-transaction-start
{
...
}

Actually, it reads better to me than having trace in front. If the above
macro name is acceptable, I'll take this route.

Regards,
-Robert

Peter Eisentraut wrote:
> I fixed that part. Note again that a buildfarm animal testing the dtrace
> support could be helpful. :)

Hi!

Our testing on Solaris Nevada (x86 and SPARC) and Solaris 10 (SPARC) in
the build farm now runs with --enable-dtrace.

Thanks for the tip!

-J
--

Jørgen Austvik, Software Engineering - QA
Sun Microsystems Database Technology Group

Attached is the updated patch which addressed all the concerns from
Peter and Tom.

* The header file containing the probe macros is now included only in
the .c files that need it.
* All probe macro names now start with TRACE_ (eg:
TRACE_POSTGRESQL_TRANSACTION_START).

Regards,
-Robert

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------

Robert Lor wrote:
>
> Attached is the updated patch which addressed all the concerns from
> Peter and Tom.
>
> * The header file containing the probe macros is now included only in
> the .c files that need it.
> * All probe macro names now start with TRACE_ (eg:
> TRACE_POSTGRESQL_TRANSACTION_START).
>
> Regards,
> -Robert

>
> --
> Sent via pgsql-patches mailing list (pgsql-patches(at)postgresql(dot)org)
> To make changes to your subscription:
> http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.org&extra=pgsql-patches

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Hi Peter,

Robert Lor wrote:
>
> Attached is the updated patch which addressed all the concerns from
> Peter and Tom.
>
>
I believe you're the reviewer of this patch. Any idea when it will be
applied? As far as I'm concerned, all the issues that were raised have
been addressed in the latest patch I sent.

Regards,
-Robert

On Wed, Feb 27, 2008 at 8:32 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> Robert Lor wrote:
> > 3) Note on src/backend/Makefile
> > The current rule below does not work. After expansion, utils/probes.d
> > needs
> > to come right after -s, but currently it shows up at the end after
> > all the .o files.
>
> I fixed that part. Note again that a buildfarm animal testing the dtrace
> support could be helpful. :)

I've now added an OS X Leopard buildfarm member (antelope - not sure
if there is a pun intended there). Once the DTrace support for Mac is
committed I'll enable it on that machine (please nudge me if I miss
it).

--
Dave Page
EnterpriseDB UK Ltd: http://www.enterprisedb.com
PostgreSQL UK 2008 Conference: http://www.postgresql.org.uk

Am Donnerstag, 6. März 2008 schrieb Robert Lor:
> Attached is the updated patch which addressed all the concerns from
> Peter and Tom.
>
> * The header file containing the probe macros is now included only in
> the .c files that need it.
> * All probe macro names now start with TRACE_ (eg:
> TRACE_POSTGRESQL_TRANSACTION_START).

Patch applied. I also added a small sed script that generates the dummy
probes.h file automatically, so it doesn't need to be manually maintained.