Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-03-17 Thread Peter Eisentraut
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:

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-03-14 Thread Dave Page
On Wed, Feb 27, 2008 at 8:32 PM, Peter Eisentraut [EMAIL PROTECTED] 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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-03-13 Thread Robert Lor
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-03-06 Thread Bruce Momjian
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. ---

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-03-05 Thread Jorgen Austvik - Sun Norway
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-03-05 Thread 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). Regards, -Robert

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Robert Lor
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Alvaro Herrera
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Robert Lor
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Robert Lor
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Alvaro Herrera
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Tom Lane
Robert Lor [EMAIL PROTECTED] 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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Tom Lane
Alvaro Herrera [EMAIL PROTECTED] 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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Robert Lor
Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] 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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Tom Lane
Robert Lor [EMAIL PROTECTED] 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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Peter Eisentraut
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Peter Eisentraut
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Robert Lor
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Tom Lane
Robert Lor [EMAIL PROTECTED] 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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Robert Lor
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,

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Alvaro Herrera
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Robert Lor
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-29 Thread Robert Lor
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-28 Thread Robert Lor
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-28 Thread Peter Eisentraut
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-28 Thread Robert Lor
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-28 Thread Tom Lane
Robert Lor [EMAIL PROTECTED] 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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-28 Thread Peter Eisentraut
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-27 Thread Alvaro Herrera
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)

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-27 Thread Robert Lor
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,

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-27 Thread Peter Eisentraut
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-27 Thread Peter Eisentraut
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-27 Thread Peter Eisentraut
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.

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-27 Thread Robert Lor
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

Re: [PATCHES] DTrace probe patch for OS X Leopard

2008-02-27 Thread Robert Lor
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