DTrace probe patch for OS X Leopard

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.Lor@Sun.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@commandprompt.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.Lor@Sun.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