Thread: 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

? src/include/probes_null.h
Index: src/Makefile
===================================================================
RCS file: /projects/cvsroot/pgsql/src/Makefile,v
retrieving revision 1.42
diff -c -r1.42 Makefile
*** src/Makefile    21 Aug 2007 01:11:12 -0000    1.42
--- src/Makefile    27 Feb 2008 03:22:55 -0000
***************
*** 14,19 ****
--- 14,22 ----


  all install installdirs uninstall distprep:
+ ifeq ($(enable_dtrace), yes)
+     $(DTRACE) -h -s $(top_builddir)/src/backend/utils/probes.d -o $(top_builddir)/src/include/probes.h
+ endif
      $(MAKE) -C port $@
      $(MAKE) -C timezone $@
      $(MAKE) -C backend $@
Index: src/backend/Makefile
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/Makefile,v
retrieving revision 1.127
diff -c -r1.127 Makefile
*** src/backend/Makefile    26 Feb 2008 14:42:27 -0000    1.127
--- src/backend/Makefile    27 Feb 2008 03:22:55 -0000
***************
*** 20,28 ****
--- 20,30 ----

  include $(srcdir)/common.mk

+ ifeq ($(PORTNAME), solaris)
  ifeq ($(enable_dtrace), yes)
  LOCALOBJS += utils/probes.o
  endif
+ endif

  OBJS = $(SUBDIROBJS) $(LOCALOBJS) $(top_builddir)/src/port/libpgport_srv.a

***************
*** 135,144 ****
      cd $(dir $@) && rm -f $(notdir $@) && \
          $(LN_S) ../../../$(subdir)/utils/fmgroids.h .

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

  ##########################################################################

--- 137,146 ----
      cd $(dir $@) && rm -f $(notdir $@) && \
          $(LN_S) ../../../$(subdir)/utils/fmgroids.h .

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

  ##########################################################################

Index: src/backend/access/transam/xact.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/xact.c,v
retrieving revision 1.257
diff -c -r1.257 xact.c
*** src/backend/access/transam/xact.c    15 Jan 2008 18:56:59 -0000    1.257
--- src/backend/access/transam/xact.c    27 Feb 2008 03:22:56 -0000
***************
*** 1479,1485 ****
      Assert(MyProc->backendId == vxid.backendId);
      MyProc->lxid = vxid.localTransactionId;

!     PG_TRACE1(transaction__start, vxid.localTransactionId);

      /*
       * set transaction_timestamp() (a/k/a now()).  We want this to be the same
--- 1479,1485 ----
      Assert(MyProc->backendId == vxid.backendId);
      MyProc->lxid = vxid.localTransactionId;

!     POSTGRESQL_TRANSACTION_START(vxid.localTransactionId);

      /*
       * set transaction_timestamp() (a/k/a now()).  We want this to be the same
***************
*** 1604,1610 ****
       */
      latestXid = RecordTransactionCommit();

!     PG_TRACE1(transaction__commit, MyProc->lxid);

      /*
       * Let others know about no transaction in progress by me. Note that this
--- 1604,1610 ----
       */
      latestXid = RecordTransactionCommit();

!     POSTGRESQL_TRANSACTION_COMMIT(MyProc->lxid);

      /*
       * Let others know about no transaction in progress by me. Note that this
***************
*** 1990,1996 ****
       */
      latestXid = RecordTransactionAbort(false);

!     PG_TRACE1(transaction__abort, MyProc->lxid);

      /*
       * Let others know about no transaction in progress by me. Note that this
--- 1990,1996 ----
       */
      latestXid = RecordTransactionAbort(false);

!     POSTGRESQL_TRANSACTION_ABORT(MyProc->lxid);

      /*
       * Let others know about no transaction in progress by me. Note that this
Index: src/backend/storage/lmgr/lock.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/storage/lmgr/lock.c,v
retrieving revision 1.181
diff -c -r1.181 lock.c
*** src/backend/storage/lmgr/lock.c    2 Feb 2008 22:26:17 -0000    1.181
--- src/backend/storage/lmgr/lock.c    27 Feb 2008 03:22:56 -0000
***************
*** 787,797 ****
           * Sleep till someone wakes me up.
           */

!         PG_TRACE2(lock__startwait, locktag->locktag_field2, lockmode);

          WaitOnLock(locallock, owner);

!         PG_TRACE2(lock__endwait, locktag->locktag_field2, lockmode);

          /*
           * NOTE: do not do any material change of state between here and
--- 787,797 ----
           * Sleep till someone wakes me up.
           */

!         POSTGRESQL_LOCK_STARTWAIT(locktag->locktag_field2, lockmode);

          WaitOnLock(locallock, owner);

!         POSTGRESQL_LOCK_ENDWAIT(locktag->locktag_field2, lockmode);

          /*
           * NOTE: do not do any material change of state between here and
Index: src/backend/storage/lmgr/lwlock.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/storage/lmgr/lwlock.c,v
retrieving revision 1.50
diff -c -r1.50 lwlock.c
*** src/backend/storage/lmgr/lwlock.c    1 Jan 2008 19:45:52 -0000    1.50
--- src/backend/storage/lmgr/lwlock.c    27 Feb 2008 03:22:57 -0000
***************
*** 447,453 ****
          block_counts[lockid]++;
  #endif

!         PG_TRACE2(lwlock__startwait, lockid, mode);

          for (;;)
          {
--- 447,453 ----
          block_counts[lockid]++;
  #endif

!         POSTGRESQL_LWLOCK_STARTWAIT(lockid, mode);

          for (;;)
          {
***************
*** 458,464 ****
              extraWaits++;
          }

!         PG_TRACE2(lwlock__endwait, lockid, mode);

          LOG_LWDEBUG("LWLockAcquire", lockid, "awakened");

--- 458,464 ----
              extraWaits++;
          }

!         POSTGRESQL_LWLOCK_ENDWAIT(lockid, mode);

          LOG_LWDEBUG("LWLockAcquire", lockid, "awakened");

***************
*** 469,475 ****
      /* We are done updating shared state of the lock itself. */
      SpinLockRelease(&lock->mutex);

!     PG_TRACE2(lwlock__acquire, lockid, mode);

      /* Add lock to list of locks held by this backend */
      held_lwlocks[num_held_lwlocks++] = lockid;
--- 469,475 ----
      /* We are done updating shared state of the lock itself. */
      SpinLockRelease(&lock->mutex);

!     POSTGRESQL_LWLOCK_ACQUIRE(lockid, mode);

      /* Add lock to list of locks held by this backend */
      held_lwlocks[num_held_lwlocks++] = lockid;
***************
*** 540,552 ****
          /* Failed to get lock, so release interrupt holdoff */
          RESUME_INTERRUPTS();
          LOG_LWDEBUG("LWLockConditionalAcquire", lockid, "failed");
!         PG_TRACE2(lwlock__condacquire__fail, lockid, mode);
      }
      else
      {
          /* Add lock to list of locks held by this backend */
          held_lwlocks[num_held_lwlocks++] = lockid;
!         PG_TRACE2(lwlock__condacquire, lockid, mode);
      }

      return !mustwait;
--- 540,552 ----
          /* Failed to get lock, so release interrupt holdoff */
          RESUME_INTERRUPTS();
          LOG_LWDEBUG("LWLockConditionalAcquire", lockid, "failed");
!         POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(lockid, mode);
      }
      else
      {
          /* Add lock to list of locks held by this backend */
          held_lwlocks[num_held_lwlocks++] = lockid;
!         POSTGRESQL_LWLOCK_CONDACQUIRE(lockid, mode);
      }

      return !mustwait;
***************
*** 631,637 ****
      /* We are done updating shared state of the lock itself. */
      SpinLockRelease(&lock->mutex);

!     PG_TRACE1(lwlock__release, lockid);

      /*
       * Awaken any waiters I removed from the queue.
--- 631,637 ----
      /* We are done updating shared state of the lock itself. */
      SpinLockRelease(&lock->mutex);

!     POSTGRESQL_LWLOCK_RELEASE(lockid);

      /*
       * Awaken any waiters I removed from the queue.
Index: src/include/c.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/c.h,v
retrieving revision 1.223
diff -c -r1.223 c.h
*** src/include/c.h    23 Feb 2008 19:11:45 -0000    1.223
--- src/include/c.h    27 Feb 2008 03:22:57 -0000
***************
*** 57,63 ****
  #include "pg_config_os.h"        /* must be before any system header files */
  #endif
  #include "postgres_ext.h"
! #include "pg_trace.h"

  #if _MSC_VER >= 1400
  #define errcode __msvc_errcode
--- 57,67 ----
  #include "pg_config_os.h"        /* must be before any system header files */
  #endif
  #include "postgres_ext.h"
! #ifdef ENABLE_DTRACE
! #include "probes.h"
! #else
! #include "probes_null.h"
! #endif

  #if _MSC_VER >= 1400
  #define errcode __msvc_errcode
/* ----------
 *      probes_null.h
 *
 *      Definitions of probe macros used when DTrace is not enabled.
 *
 *      Copyright (c) 2006-2008, PostgreSQL Global Development Group
 *
 * ----------
 */

#ifndef    _PROBES_NULL_H
#define    _PROBES_NULL_H

#define    POSTGRESQL_LOCK_ENDWAIT(arg0, arg1)
#define    POSTGRESQL_LOCK_ENDWAIT_ENABLED()

#define    POSTGRESQL_LOCK_STARTWAIT(arg0, arg1)
#define    POSTGRESQL_LOCK_STARTWAIT_ENABLED()

#define    POSTGRESQL_LWLOCK_ACQUIRE(arg0, arg1)
#define    POSTGRESQL_LWLOCK_ACQUIRE_ENABLED()

#define    POSTGRESQL_LWLOCK_CONDACQUIRE(arg0, arg1)
#define    POSTGRESQL_LWLOCK_CONDACQUIRE_ENABLED()

#define    POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(arg0, arg1)
#define    POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL_ENABLED()

#define    POSTGRESQL_LWLOCK_ENDWAIT(arg0, arg1)
#define    POSTGRESQL_LWLOCK_ENDWAIT_ENABLED()

#define    POSTGRESQL_LWLOCK_RELEASE(arg0)
#define    POSTGRESQL_LWLOCK_RELEASE_ENABLED()

#define    POSTGRESQL_LWLOCK_STARTWAIT(arg0, arg1)
#define    POSTGRESQL_LWLOCK_STARTWAIT_ENABLED()

#define    POSTGRESQL_TRANSACTION_ABORT(arg0)
#define    POSTGRESQL_TRANSACTION_ABORT_ENABLED()

#define    POSTGRESQL_TRANSACTION_COMMIT(arg0)
#define    POSTGRESQL_TRANSACTION_COMMIT_ENABLED()

#define    POSTGRESQL_TRANSACTION_START(arg0)
#define    POSTGRESQL_TRANSACTION_START_ENABLED()

#endif    /* _PROBES_NULL_H */

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

? src/include/probes_null.h
Index: src/Makefile
===================================================================
RCS file: /projects/cvsroot/pgsql/src/Makefile,v
retrieving revision 1.42
diff -c -r1.42 Makefile
*** src/Makefile    21 Aug 2007 01:11:12 -0000    1.42
--- src/Makefile    27 Feb 2008 17:09:41 -0000
***************
*** 14,19 ****
--- 14,22 ----


  all install installdirs uninstall distprep:
+ ifeq ($(enable_dtrace), yes)
+     $(DTRACE) -h -s $(top_builddir)/src/backend/utils/probes.d -o $(top_builddir)/src/include/probes.h
+ endif
      $(MAKE) -C port $@
      $(MAKE) -C timezone $@
      $(MAKE) -C backend $@
Index: src/backend/Makefile
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/Makefile,v
retrieving revision 1.127
diff -c -r1.127 Makefile
*** src/backend/Makefile    26 Feb 2008 14:42:27 -0000    1.127
--- src/backend/Makefile    27 Feb 2008 17:09:41 -0000
***************
*** 20,28 ****
--- 20,30 ----

  include $(srcdir)/common.mk

+ ifeq ($(PORTNAME), solaris)
  ifeq ($(enable_dtrace), yes)
  LOCALOBJS += utils/probes.o
  endif
+ endif

  OBJS = $(SUBDIROBJS) $(LOCALOBJS) $(top_builddir)/src/port/libpgport_srv.a

***************
*** 135,144 ****
      cd $(dir $@) && rm -f $(notdir $@) && \
          $(LN_S) ../../../$(subdir)/utils/fmgroids.h .

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

  ##########################################################################

--- 137,146 ----
      cd $(dir $@) && rm -f $(notdir $@) && \
          $(LN_S) ../../../$(subdir)/utils/fmgroids.h .

! ifeq ($(PORTNAME), solaris)
  utils/probes.o: utils/probes.d $(SUBDIROBJS)
!     $(DTRACE) $(DTRACEFLAGS) -G -s $< $(filter-out $<, $(call expand_subsys,$^)) -o $@
! endif

  ##########################################################################

Index: src/backend/access/transam/xact.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/xact.c,v
retrieving revision 1.257
diff -c -r1.257 xact.c
*** src/backend/access/transam/xact.c    15 Jan 2008 18:56:59 -0000    1.257
--- src/backend/access/transam/xact.c    27 Feb 2008 17:09:41 -0000
***************
*** 1479,1485 ****
      Assert(MyProc->backendId == vxid.backendId);
      MyProc->lxid = vxid.localTransactionId;

!     PG_TRACE1(transaction__start, vxid.localTransactionId);

      /*
       * set transaction_timestamp() (a/k/a now()).  We want this to be the same
--- 1479,1485 ----
      Assert(MyProc->backendId == vxid.backendId);
      MyProc->lxid = vxid.localTransactionId;

!     POSTGRESQL_TRANSACTION_START(vxid.localTransactionId);

      /*
       * set transaction_timestamp() (a/k/a now()).  We want this to be the same
***************
*** 1604,1610 ****
       */
      latestXid = RecordTransactionCommit();

!     PG_TRACE1(transaction__commit, MyProc->lxid);

      /*
       * Let others know about no transaction in progress by me. Note that this
--- 1604,1610 ----
       */
      latestXid = RecordTransactionCommit();

!     POSTGRESQL_TRANSACTION_COMMIT(MyProc->lxid);

      /*
       * Let others know about no transaction in progress by me. Note that this
***************
*** 1990,1996 ****
       */
      latestXid = RecordTransactionAbort(false);

!     PG_TRACE1(transaction__abort, MyProc->lxid);

      /*
       * Let others know about no transaction in progress by me. Note that this
--- 1990,1996 ----
       */
      latestXid = RecordTransactionAbort(false);

!     POSTGRESQL_TRANSACTION_ABORT(MyProc->lxid);

      /*
       * Let others know about no transaction in progress by me. Note that this
Index: src/backend/storage/lmgr/lock.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/storage/lmgr/lock.c,v
retrieving revision 1.181
diff -c -r1.181 lock.c
*** src/backend/storage/lmgr/lock.c    2 Feb 2008 22:26:17 -0000    1.181
--- src/backend/storage/lmgr/lock.c    27 Feb 2008 17:09:42 -0000
***************
*** 787,797 ****
           * Sleep till someone wakes me up.
           */

!         PG_TRACE2(lock__startwait, locktag->locktag_field2, lockmode);

          WaitOnLock(locallock, owner);

!         PG_TRACE2(lock__endwait, locktag->locktag_field2, lockmode);

          /*
           * NOTE: do not do any material change of state between here and
--- 787,797 ----
           * Sleep till someone wakes me up.
           */

!         POSTGRESQL_LOCK_STARTWAIT(locktag->locktag_field2, lockmode);

          WaitOnLock(locallock, owner);

!         POSTGRESQL_LOCK_ENDWAIT(locktag->locktag_field2, lockmode);

          /*
           * NOTE: do not do any material change of state between here and
Index: src/backend/storage/lmgr/lwlock.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/storage/lmgr/lwlock.c,v
retrieving revision 1.50
diff -c -r1.50 lwlock.c
*** src/backend/storage/lmgr/lwlock.c    1 Jan 2008 19:45:52 -0000    1.50
--- src/backend/storage/lmgr/lwlock.c    27 Feb 2008 17:09:42 -0000
***************
*** 447,453 ****
          block_counts[lockid]++;
  #endif

!         PG_TRACE2(lwlock__startwait, lockid, mode);

          for (;;)
          {
--- 447,453 ----
          block_counts[lockid]++;
  #endif

!         POSTGRESQL_LWLOCK_STARTWAIT(lockid, mode);

          for (;;)
          {
***************
*** 458,464 ****
              extraWaits++;
          }

!         PG_TRACE2(lwlock__endwait, lockid, mode);

          LOG_LWDEBUG("LWLockAcquire", lockid, "awakened");

--- 458,464 ----
              extraWaits++;
          }

!         POSTGRESQL_LWLOCK_ENDWAIT(lockid, mode);

          LOG_LWDEBUG("LWLockAcquire", lockid, "awakened");

***************
*** 469,475 ****
      /* We are done updating shared state of the lock itself. */
      SpinLockRelease(&lock->mutex);

!     PG_TRACE2(lwlock__acquire, lockid, mode);

      /* Add lock to list of locks held by this backend */
      held_lwlocks[num_held_lwlocks++] = lockid;
--- 469,475 ----
      /* We are done updating shared state of the lock itself. */
      SpinLockRelease(&lock->mutex);

!     POSTGRESQL_LWLOCK_ACQUIRE(lockid, mode);

      /* Add lock to list of locks held by this backend */
      held_lwlocks[num_held_lwlocks++] = lockid;
***************
*** 540,552 ****
          /* Failed to get lock, so release interrupt holdoff */
          RESUME_INTERRUPTS();
          LOG_LWDEBUG("LWLockConditionalAcquire", lockid, "failed");
!         PG_TRACE2(lwlock__condacquire__fail, lockid, mode);
      }
      else
      {
          /* Add lock to list of locks held by this backend */
          held_lwlocks[num_held_lwlocks++] = lockid;
!         PG_TRACE2(lwlock__condacquire, lockid, mode);
      }

      return !mustwait;
--- 540,552 ----
          /* Failed to get lock, so release interrupt holdoff */
          RESUME_INTERRUPTS();
          LOG_LWDEBUG("LWLockConditionalAcquire", lockid, "failed");
!         POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(lockid, mode);
      }
      else
      {
          /* Add lock to list of locks held by this backend */
          held_lwlocks[num_held_lwlocks++] = lockid;
!         POSTGRESQL_LWLOCK_CONDACQUIRE(lockid, mode);
      }

      return !mustwait;
***************
*** 631,637 ****
      /* We are done updating shared state of the lock itself. */
      SpinLockRelease(&lock->mutex);

!     PG_TRACE1(lwlock__release, lockid);

      /*
       * Awaken any waiters I removed from the queue.
--- 631,637 ----
      /* We are done updating shared state of the lock itself. */
      SpinLockRelease(&lock->mutex);

!     POSTGRESQL_LWLOCK_RELEASE(lockid);

      /*
       * Awaken any waiters I removed from the queue.
Index: src/include/c.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/c.h,v
retrieving revision 1.223
diff -c -r1.223 c.h
*** src/include/c.h    23 Feb 2008 19:11:45 -0000    1.223
--- src/include/c.h    27 Feb 2008 17:09:42 -0000
***************
*** 57,63 ****
  #include "pg_config_os.h"        /* must be before any system header files */
  #endif
  #include "postgres_ext.h"
! #include "pg_trace.h"

  #if _MSC_VER >= 1400
  #define errcode __msvc_errcode
--- 57,67 ----
  #include "pg_config_os.h"        /* must be before any system header files */
  #endif
  #include "postgres_ext.h"
! #ifdef ENABLE_DTRACE
! #include "probes.h"
! #else
! #include "probes_null.h"
! #endif

  #if _MSC_VER >= 1400
  #define errcode __msvc_errcode

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
/* ----------
 *      probes_null.h
 *
 *      Definitions of probe macros used when DTrace is not enabled.
 *
 *      Copyright (c) 2006-2008, PostgreSQL Global Development Group
 *
 * ----------
 */

#ifndef    _PROBES_NULL_H
#define    _PROBES_NULL_H

#define    POSTGRESQL_LOCK_ENDWAIT(arg0, arg1)
#define    POSTGRESQL_LOCK_ENDWAIT_ENABLED()

#define    POSTGRESQL_LOCK_STARTWAIT(arg0, arg1)
#define    POSTGRESQL_LOCK_STARTWAIT_ENABLED()

#define    POSTGRESQL_LWLOCK_ACQUIRE(arg0, arg1)
#define    POSTGRESQL_LWLOCK_ACQUIRE_ENABLED()

#define    POSTGRESQL_LWLOCK_CONDACQUIRE(arg0, arg1)
#define    POSTGRESQL_LWLOCK_CONDACQUIRE_ENABLED()

#define    POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(arg0, arg1)
#define    POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL_ENABLED()

#define    POSTGRESQL_LWLOCK_ENDWAIT(arg0, arg1)
#define    POSTGRESQL_LWLOCK_ENDWAIT_ENABLED()

#define    POSTGRESQL_LWLOCK_RELEASE(arg0)
#define    POSTGRESQL_LWLOCK_RELEASE_ENABLED()

#define    POSTGRESQL_LWLOCK_STARTWAIT(arg0, arg1)
#define    POSTGRESQL_LWLOCK_STARTWAIT_ENABLED()

#define    POSTGRESQL_TRANSACTION_ABORT(arg0)
#define    POSTGRESQL_TRANSACTION_ABORT_ENABLED()

#define    POSTGRESQL_TRANSACTION_COMMIT(arg0)
#define    POSTGRESQL_TRANSACTION_COMMIT_ENABLED()

#define    POSTGRESQL_TRANSACTION_START(arg0)
#define    POSTGRESQL_TRANSACTION_START_ENABLED()

#endif    /* _PROBES_NULL_H */

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
Index: src/Makefile
===================================================================
RCS file: /projects/cvsroot/pgsql/src/Makefile,v
retrieving revision 1.42
diff -u -3 -p -r1.42 Makefile
--- src/Makefile    21 Aug 2007 01:11:12 -0000    1.42
+++ src/Makefile    28 Feb 2008 21:41:49 -0000
@@ -14,6 +14,9 @@ include Makefile.global


 all install installdirs uninstall distprep:
+ifeq ($(enable_dtrace), yes)
+    $(MAKE) -C backend ../../src/include/utils/probes.h
+endif
     $(MAKE) -C port $@
     $(MAKE) -C timezone $@
     $(MAKE) -C backend $@
Index: src/backend/Makefile
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/Makefile,v
retrieving revision 1.127
diff -u -3 -p -r1.127 Makefile
--- src/backend/Makefile    26 Feb 2008 14:42:27 -0000    1.127
+++ src/backend/Makefile    28 Feb 2008 21:41:49 -0000
@@ -20,9 +20,11 @@ SUBDIRS = access bootstrap catalog parse

 include $(srcdir)/common.mk

+ifeq ($(PORTNAME), solaris)
 ifeq ($(enable_dtrace), yes)
 LOCALOBJS += utils/probes.o
 endif
+endif

 OBJS = $(SUBDIROBJS) $(LOCALOBJS) $(top_builddir)/src/port/libpgport_srv.a

@@ -122,6 +124,9 @@ $(srcdir)/parser/parse.h: parser/gram.y
 utils/fmgroids.h: utils/Gen_fmgrtab.sh $(top_srcdir)/src/include/catalog/pg_proc.h
     $(MAKE) -C utils fmgroids.h

+utils/probes.h: utils/probes.d
+    $(MAKE) -C utils probes.h
+
 # Make symlinks for these headers in the include directory. That way
 # we can cut down on the -I options. Also, a symlink is automatically
 # up to date when we update the base file.
@@ -135,9 +140,15 @@ $(top_builddir)/src/include/utils/fmgroi
     cd $(dir $@) && rm -f $(notdir $@) && \
         $(LN_S) ../../../$(subdir)/utils/fmgroids.h .

+$(top_builddir)/src/include/utils/probes.h: utils/probes.h
+    cd $(dir $@) && rm -f $(notdir $@) && \
+        $(LN_S) ../../../$(subdir)/utils/probes.h .

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


 ##########################################################################
Index: src/backend/access/transam/xact.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/xact.c,v
retrieving revision 1.257
diff -u -3 -p -r1.257 xact.c
--- src/backend/access/transam/xact.c    15 Jan 2008 18:56:59 -0000    1.257
+++ src/backend/access/transam/xact.c    28 Feb 2008 21:41:49 -0000
@@ -1479,7 +1479,7 @@ StartTransaction(void)
     Assert(MyProc->backendId == vxid.backendId);
     MyProc->lxid = vxid.localTransactionId;

-    PG_TRACE1(transaction__start, vxid.localTransactionId);
+    POSTGRESQL_TRANSACTION_START(vxid.localTransactionId);

     /*
      * set transaction_timestamp() (a/k/a now()).  We want this to be the same
@@ -1604,7 +1604,7 @@ CommitTransaction(void)
      */
     latestXid = RecordTransactionCommit();

-    PG_TRACE1(transaction__commit, MyProc->lxid);
+    POSTGRESQL_TRANSACTION_COMMIT(MyProc->lxid);

     /*
      * Let others know about no transaction in progress by me. Note that this
@@ -1990,7 +1990,7 @@ AbortTransaction(void)
      */
     latestXid = RecordTransactionAbort(false);

-    PG_TRACE1(transaction__abort, MyProc->lxid);
+    POSTGRESQL_TRANSACTION_ABORT(MyProc->lxid);

     /*
      * Let others know about no transaction in progress by me. Note that this
Index: src/backend/storage/lmgr/lock.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/storage/lmgr/lock.c,v
retrieving revision 1.181
diff -u -3 -p -r1.181 lock.c
--- src/backend/storage/lmgr/lock.c    2 Feb 2008 22:26:17 -0000    1.181
+++ src/backend/storage/lmgr/lock.c    28 Feb 2008 21:41:50 -0000
@@ -787,11 +787,11 @@ LockAcquire(const LOCKTAG *locktag,
          * Sleep till someone wakes me up.
          */

-        PG_TRACE2(lock__startwait, locktag->locktag_field2, lockmode);
+        POSTGRESQL_LOCK_STARTWAIT(locktag->locktag_field2, lockmode);

         WaitOnLock(locallock, owner);

-        PG_TRACE2(lock__endwait, locktag->locktag_field2, lockmode);
+        POSTGRESQL_LOCK_ENDWAIT(locktag->locktag_field2, lockmode);

         /*
          * NOTE: do not do any material change of state between here and
Index: src/backend/storage/lmgr/lwlock.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/storage/lmgr/lwlock.c,v
retrieving revision 1.50
diff -u -3 -p -r1.50 lwlock.c
--- src/backend/storage/lmgr/lwlock.c    1 Jan 2008 19:45:52 -0000    1.50
+++ src/backend/storage/lmgr/lwlock.c    28 Feb 2008 21:41:50 -0000
@@ -447,7 +447,7 @@ LWLockAcquire(LWLockId lockid, LWLockMod
         block_counts[lockid]++;
 #endif

-        PG_TRACE2(lwlock__startwait, lockid, mode);
+        POSTGRESQL_LWLOCK_STARTWAIT(lockid, mode);

         for (;;)
         {
@@ -458,7 +458,7 @@ LWLockAcquire(LWLockId lockid, LWLockMod
             extraWaits++;
         }

-        PG_TRACE2(lwlock__endwait, lockid, mode);
+        POSTGRESQL_LWLOCK_ENDWAIT(lockid, mode);

         LOG_LWDEBUG("LWLockAcquire", lockid, "awakened");

@@ -469,7 +469,7 @@ LWLockAcquire(LWLockId lockid, LWLockMod
     /* We are done updating shared state of the lock itself. */
     SpinLockRelease(&lock->mutex);

-    PG_TRACE2(lwlock__acquire, lockid, mode);
+    POSTGRESQL_LWLOCK_ACQUIRE(lockid, mode);

     /* Add lock to list of locks held by this backend */
     held_lwlocks[num_held_lwlocks++] = lockid;
@@ -540,13 +540,13 @@ LWLockConditionalAcquire(LWLockId lockid
         /* Failed to get lock, so release interrupt holdoff */
         RESUME_INTERRUPTS();
         LOG_LWDEBUG("LWLockConditionalAcquire", lockid, "failed");
-        PG_TRACE2(lwlock__condacquire__fail, lockid, mode);
+        POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(lockid, mode);
     }
     else
     {
         /* Add lock to list of locks held by this backend */
         held_lwlocks[num_held_lwlocks++] = lockid;
-        PG_TRACE2(lwlock__condacquire, lockid, mode);
+        POSTGRESQL_LWLOCK_CONDACQUIRE(lockid, mode);
     }

     return !mustwait;
@@ -631,7 +631,7 @@ LWLockRelease(LWLockId lockid)
     /* We are done updating shared state of the lock itself. */
     SpinLockRelease(&lock->mutex);

-    PG_TRACE1(lwlock__release, lockid);
+    POSTGRESQL_LWLOCK_RELEASE(lockid);

     /*
      * Awaken any waiters I removed from the queue.
Index: src/backend/utils/Makefile
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/Makefile,v
retrieving revision 1.26
diff -u -3 -p -r1.26 Makefile
--- src/backend/utils/Makefile    19 Feb 2008 10:30:08 -0000    1.26
+++ src/backend/utils/Makefile    28 Feb 2008 21:41:50 -0000
@@ -13,12 +13,23 @@ SUBDIRS     = adt cache error fmgr hash

 include $(top_srcdir)/src/backend/common.mk

+ifeq ($(enable_dtrace), yes)
+all: fmgroids.h probes.h
+else
 all: fmgroids.h
+endif

 $(SUBDIRS:%=%-recursive): fmgroids.h

 fmgroids.h fmgrtab.c: Gen_fmgrtab.sh $(top_srcdir)/src/include/catalog/pg_proc.h
     AWK='$(AWK)' $(SHELL) $< $(top_srcdir)/src/include/catalog/pg_proc.h

+
+ifeq ($(enable_dtrace), yes)
+probes.h: probes.d
+    $(DTRACE) -h -s $< -o $@
+endif
+
+
 clean:
-    rm -f fmgroids.h fmgrtab.c
+    rm -f fmgroids.h fmgrtab.c probes.h
Index: src/include/Makefile
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/Makefile,v
retrieving revision 1.23
diff -u -3 -p -r1.23 Makefile
--- src/include/Makefile    14 Oct 2007 17:07:51 -0000    1.23
+++ src/include/Makefile    28 Feb 2008 21:41:50 -0000
@@ -60,7 +60,7 @@ uninstall:


 clean:
-    rm -f utils/fmgroids.h parser/parse.h
+    rm -f utils/fmgroids.h parser/parse.h utils/probes.h

 distclean maintainer-clean: clean
     rm -f pg_config.h dynloader.h pg_config_os.h stamp-h
Index: src/include/c.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/c.h,v
retrieving revision 1.223
diff -u -3 -p -r1.223 c.h
--- src/include/c.h    23 Feb 2008 19:11:45 -0000    1.223
+++ src/include/c.h    28 Feb 2008 21:41:50 -0000
@@ -57,7 +57,11 @@
 #include "pg_config_os.h"        /* must be before any system header files */
 #endif
 #include "postgres_ext.h"
-#include "pg_trace.h"
+#ifdef ENABLE_DTRACE
+#include "utils/probes.h"
+#else
+#include "utils/probes_null.h"
+#endif

 #if _MSC_VER >= 1400
 #define errcode __msvc_errcode
Index: src/include/utils/probes_null.h
===================================================================
RCS file: src/include/utils/probes_null.h
diff -N src/include/utils/probes_null.h
--- /dev/null    1 Jan 1970 00:00:00 -0000
+++ src/include/utils/probes_null.h    28 Feb 2008 21:41:50 -0000
@@ -0,0 +1,47 @@
+/* ----------
+ *      probes_null.h
+ *
+ *      Definitions of probe macros used when DTrace is not enabled.
+ *
+ *      Copyright (c) 2006-2008, PostgreSQL Global Development Group
+ *
+ * ----------
+ */
+
+#ifndef    _PROBES_NULL_H
+#define    _PROBES_NULL_H
+
+#define    POSTGRESQL_LOCK_ENDWAIT(arg0, arg1)
+#define    POSTGRESQL_LOCK_ENDWAIT_ENABLED()
+
+#define    POSTGRESQL_LOCK_STARTWAIT(arg0, arg1)
+#define    POSTGRESQL_LOCK_STARTWAIT_ENABLED()
+
+#define    POSTGRESQL_LWLOCK_ACQUIRE(arg0, arg1)
+#define    POSTGRESQL_LWLOCK_ACQUIRE_ENABLED()
+
+#define    POSTGRESQL_LWLOCK_CONDACQUIRE(arg0, arg1)
+#define    POSTGRESQL_LWLOCK_CONDACQUIRE_ENABLED()
+
+#define    POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(arg0, arg1)
+#define    POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL_ENABLED()
+
+#define    POSTGRESQL_LWLOCK_ENDWAIT(arg0, arg1)
+#define    POSTGRESQL_LWLOCK_ENDWAIT_ENABLED()
+
+#define    POSTGRESQL_LWLOCK_RELEASE(arg0)
+#define    POSTGRESQL_LWLOCK_RELEASE_ENABLED()
+
+#define    POSTGRESQL_LWLOCK_STARTWAIT(arg0, arg1)
+#define    POSTGRESQL_LWLOCK_STARTWAIT_ENABLED()
+
+#define    POSTGRESQL_TRANSACTION_ABORT(arg0)
+#define    POSTGRESQL_TRANSACTION_ABORT_ENABLED()
+
+#define    POSTGRESQL_TRANSACTION_COMMIT(arg0)
+#define    POSTGRESQL_TRANSACTION_COMMIT_ENABLED()
+
+#define    POSTGRESQL_TRANSACTION_START(arg0)
+#define    POSTGRESQL_TRANSACTION_START_ENABLED()
+
+#endif    /* _PROBES_NULL_H */

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

Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.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.Lor@Sun.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.Lor@Sun.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
Index: src/Makefile
===================================================================
RCS file: /projects/cvsroot/pgsql/src/Makefile,v
retrieving revision 1.42
diff -u -3 -p -r1.42 Makefile
--- src/Makefile    21 Aug 2007 01:11:12 -0000    1.42
+++ src/Makefile    6 Mar 2008 04:25:25 -0000
@@ -14,6 +14,9 @@ include Makefile.global


 all install installdirs uninstall distprep:
+ifeq ($(enable_dtrace), yes)
+    $(MAKE) -C backend ../../src/include/utils/probes.h
+endif
     $(MAKE) -C port $@
     $(MAKE) -C timezone $@
     $(MAKE) -C backend $@
Index: src/backend/Makefile
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/Makefile,v
retrieving revision 1.127
diff -u -3 -p -r1.127 Makefile
--- src/backend/Makefile    26 Feb 2008 14:42:27 -0000    1.127
+++ src/backend/Makefile    6 Mar 2008 04:25:25 -0000
@@ -20,9 +20,11 @@ SUBDIRS = access bootstrap catalog parse

 include $(srcdir)/common.mk

+ifeq ($(PORTNAME), solaris)
 ifeq ($(enable_dtrace), yes)
 LOCALOBJS += utils/probes.o
 endif
+endif

 OBJS = $(SUBDIROBJS) $(LOCALOBJS) $(top_builddir)/src/port/libpgport_srv.a

@@ -122,6 +124,9 @@ $(srcdir)/parser/parse.h: parser/gram.y
 utils/fmgroids.h: utils/Gen_fmgrtab.sh $(top_srcdir)/src/include/catalog/pg_proc.h
     $(MAKE) -C utils fmgroids.h

+utils/probes.h: utils/probes.d
+    $(MAKE) -C utils probes.h
+
 # Make symlinks for these headers in the include directory. That way
 # we can cut down on the -I options. Also, a symlink is automatically
 # up to date when we update the base file.
@@ -135,9 +140,15 @@ $(top_builddir)/src/include/utils/fmgroi
     cd $(dir $@) && rm -f $(notdir $@) && \
         $(LN_S) ../../../$(subdir)/utils/fmgroids.h .

+$(top_builddir)/src/include/utils/probes.h: utils/probes.h
+    cd $(dir $@) && rm -f $(notdir $@) && \
+        $(LN_S) ../../../$(subdir)/utils/probes.h .

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


 ##########################################################################
Index: src/backend/access/transam/xact.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/xact.c,v
retrieving revision 1.257
diff -u -3 -p -r1.257 xact.c
--- src/backend/access/transam/xact.c    15 Jan 2008 18:56:59 -0000    1.257
+++ src/backend/access/transam/xact.c    6 Mar 2008 04:25:25 -0000
@@ -46,6 +46,7 @@
 #include "utils/memutils.h"
 #include "utils/relcache.h"
 #include "utils/xml.h"
+#include "pg_trace.h"


 /*
@@ -1479,7 +1480,7 @@ StartTransaction(void)
     Assert(MyProc->backendId == vxid.backendId);
     MyProc->lxid = vxid.localTransactionId;

-    PG_TRACE1(transaction__start, vxid.localTransactionId);
+    TRACE_POSTGRESQL_TRANSACTION_START(vxid.localTransactionId);

     /*
      * set transaction_timestamp() (a/k/a now()).  We want this to be the same
@@ -1604,7 +1605,7 @@ CommitTransaction(void)
      */
     latestXid = RecordTransactionCommit();

-    PG_TRACE1(transaction__commit, MyProc->lxid);
+    TRACE_POSTGRESQL_TRANSACTION_COMMIT(MyProc->lxid);

     /*
      * Let others know about no transaction in progress by me. Note that this
@@ -1990,7 +1991,7 @@ AbortTransaction(void)
      */
     latestXid = RecordTransactionAbort(false);

-    PG_TRACE1(transaction__abort, MyProc->lxid);
+    TRACE_POSTGRESQL_TRANSACTION_ABORT(MyProc->lxid);

     /*
      * Let others know about no transaction in progress by me. Note that this
Index: src/backend/storage/lmgr/lock.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/storage/lmgr/lock.c,v
retrieving revision 1.181
diff -u -3 -p -r1.181 lock.c
--- src/backend/storage/lmgr/lock.c    2 Feb 2008 22:26:17 -0000    1.181
+++ src/backend/storage/lmgr/lock.c    6 Mar 2008 04:25:26 -0000
@@ -41,6 +41,7 @@
 #include "utils/memutils.h"
 #include "utils/ps_status.h"
 #include "utils/resowner.h"
+#include "pg_trace.h"


 /* This configuration variable is used to set the lock table size */
@@ -787,11 +788,11 @@ LockAcquire(const LOCKTAG *locktag,
          * Sleep till someone wakes me up.
          */

-        PG_TRACE2(lock__startwait, locktag->locktag_field2, lockmode);
+        TRACE_POSTGRESQL_LOCK_STARTWAIT(locktag->locktag_field2, lockmode);

         WaitOnLock(locallock, owner);

-        PG_TRACE2(lock__endwait, locktag->locktag_field2, lockmode);
+        TRACE_POSTGRESQL_LOCK_ENDWAIT(locktag->locktag_field2, lockmode);

         /*
          * NOTE: do not do any material change of state between here and
Index: src/backend/storage/lmgr/lwlock.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/storage/lmgr/lwlock.c,v
retrieving revision 1.50
diff -u -3 -p -r1.50 lwlock.c
--- src/backend/storage/lmgr/lwlock.c    1 Jan 2008 19:45:52 -0000    1.50
+++ src/backend/storage/lmgr/lwlock.c    6 Mar 2008 04:25:26 -0000
@@ -28,6 +28,7 @@
 #include "storage/ipc.h"
 #include "storage/proc.h"
 #include "storage/spin.h"
+#include "pg_trace.h"


 /* We use the ShmemLock spinlock to protect LWLockAssign */
@@ -447,7 +448,7 @@ LWLockAcquire(LWLockId lockid, LWLockMod
         block_counts[lockid]++;
 #endif

-        PG_TRACE2(lwlock__startwait, lockid, mode);
+        TRACE_POSTGRESQL_LWLOCK_STARTWAIT(lockid, mode);

         for (;;)
         {
@@ -458,7 +459,7 @@ LWLockAcquire(LWLockId lockid, LWLockMod
             extraWaits++;
         }

-        PG_TRACE2(lwlock__endwait, lockid, mode);
+        TRACE_POSTGRESQL_LWLOCK_ENDWAIT(lockid, mode);

         LOG_LWDEBUG("LWLockAcquire", lockid, "awakened");

@@ -469,7 +470,7 @@ LWLockAcquire(LWLockId lockid, LWLockMod
     /* We are done updating shared state of the lock itself. */
     SpinLockRelease(&lock->mutex);

-    PG_TRACE2(lwlock__acquire, lockid, mode);
+    TRACE_POSTGRESQL_LWLOCK_ACQUIRE(lockid, mode);

     /* Add lock to list of locks held by this backend */
     held_lwlocks[num_held_lwlocks++] = lockid;
@@ -540,13 +541,13 @@ LWLockConditionalAcquire(LWLockId lockid
         /* Failed to get lock, so release interrupt holdoff */
         RESUME_INTERRUPTS();
         LOG_LWDEBUG("LWLockConditionalAcquire", lockid, "failed");
-        PG_TRACE2(lwlock__condacquire__fail, lockid, mode);
+        TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(lockid, mode);
     }
     else
     {
         /* Add lock to list of locks held by this backend */
         held_lwlocks[num_held_lwlocks++] = lockid;
-        PG_TRACE2(lwlock__condacquire, lockid, mode);
+        TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE(lockid, mode);
     }

     return !mustwait;
@@ -631,7 +632,7 @@ LWLockRelease(LWLockId lockid)
     /* We are done updating shared state of the lock itself. */
     SpinLockRelease(&lock->mutex);

-    PG_TRACE1(lwlock__release, lockid);
+    TRACE_POSTGRESQL_LWLOCK_RELEASE(lockid);

     /*
      * Awaken any waiters I removed from the queue.
Index: src/backend/utils/Makefile
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/Makefile,v
retrieving revision 1.26
diff -u -3 -p -r1.26 Makefile
--- src/backend/utils/Makefile    19 Feb 2008 10:30:08 -0000    1.26
+++ src/backend/utils/Makefile    6 Mar 2008 04:25:26 -0000
@@ -13,12 +13,25 @@ SUBDIRS     = adt cache error fmgr hash

 include $(top_srcdir)/src/backend/common.mk

+ifeq ($(enable_dtrace), yes)
+all: fmgroids.h probes.h
+else
 all: fmgroids.h
+endif

 $(SUBDIRS:%=%-recursive): fmgroids.h

 fmgroids.h fmgrtab.c: Gen_fmgrtab.sh $(top_srcdir)/src/include/catalog/pg_proc.h
     AWK='$(AWK)' $(SHELL) $< $(top_srcdir)/src/include/catalog/pg_proc.h

+
+ifeq ($(enable_dtrace), yes)
+probes.h: probes.d
+    $(DTRACE) -h -s $< -o $@
+    sed -e "s/POSTGRESQL_/TRACE_POSTGRESQL_/g" $@ > $@.tmp
+    mv $@.tmp $@
+endif
+
+
 clean:
-    rm -f fmgroids.h fmgrtab.c
+    rm -f fmgroids.h fmgrtab.c probes.h
Index: src/include/Makefile
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/Makefile,v
retrieving revision 1.23
diff -u -3 -p -r1.23 Makefile
--- src/include/Makefile    14 Oct 2007 17:07:51 -0000    1.23
+++ src/include/Makefile    6 Mar 2008 04:25:26 -0000
@@ -60,7 +60,7 @@ uninstall:


 clean:
-    rm -f utils/fmgroids.h parser/parse.h
+    rm -f utils/fmgroids.h parser/parse.h utils/probes.h

 distclean maintainer-clean: clean
     rm -f pg_config.h dynloader.h pg_config_os.h stamp-h
Index: src/include/c.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/c.h,v
retrieving revision 1.223
diff -u -3 -p -r1.223 c.h
--- src/include/c.h    23 Feb 2008 19:11:45 -0000    1.223
+++ src/include/c.h    6 Mar 2008 04:25:26 -0000
@@ -57,7 +57,6 @@
 #include "pg_config_os.h"        /* must be before any system header files */
 #endif
 #include "postgres_ext.h"
-#include "pg_trace.h"

 #if _MSC_VER >= 1400
 #define errcode __msvc_errcode
Index: src/include/pg_trace.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/pg_trace.h,v
retrieving revision 1.3
diff -u -3 -p -r1.3 pg_trace.h
--- src/include/pg_trace.h    2 Jan 2008 02:42:06 -0000    1.3
+++ src/include/pg_trace.h    6 Mar 2008 04:25:26 -0000
@@ -14,41 +14,51 @@

 #ifdef ENABLE_DTRACE

-#include <sys/sdt.h>
+#include "utils/probes.h"
+
+#else    /* not ENABLE_DTRACE */

 /*
- * The PG_TRACE macros are mapped to the appropriate macros used by DTrace.
+ * Unless DTrace is explicitly enabled with --enable-dtrace, all the probe
+ * macros will be translated to no-ops.
  *
- * Only one DTrace provider called "postgresql" will be used for PostgreSQL,
- * so the name is hard-coded here to avoid having to specify it in the
- * source code.
+ * When adding new probes, two no-ops macros are needed for each probe.
+ * The macro names can be copied from utils/probes.h.
  */

-#define PG_TRACE(name) \
-    DTRACE_PROBE(postgresql, name)
-#define PG_TRACE1(name, arg1) \
-    DTRACE_PROBE1(postgresql, name, arg1)
-#define PG_TRACE2(name, arg1, arg2) \
-    DTRACE_PROBE2(postgresql, name, arg1, arg2)
-#define PG_TRACE3(name, arg1, arg2, arg3) \
-    DTRACE_PROBE3(postgresql, name, arg1, arg2, arg3)
-#define PG_TRACE4(name, arg1, arg2, arg3, arg4) \
-    DTRACE_PROBE4(postgresql, name, arg1, arg2, arg3, arg4)
-#define PG_TRACE5(name, arg1, arg2, arg3, arg4, arg5) \
-    DTRACE_PROBE5(postgresql, name, arg1, arg2, arg3, arg4, arg5)
-#else                            /* not ENABLE_DTRACE */
+#define    TRACE_POSTGRESQL_TRANSACTION_START(arg0)
+#define    TRACE_POSTGRESQL_TRANSACTION_START_ENABLED() (0)

-/*
- * Unless DTrace is explicitly enabled with --enable-dtrace, the PG_TRACE
- * macros will expand to no-ops.
- */
+#define    TRACE_POSTGRESQL_TRANSACTION_COMMIT(arg0)
+#define    TRACE_POSTGRESQL_TRANSACTION_COMMIT_ENABLED() (0)
+
+#define    TRACE_POSTGRESQL_TRANSACTION_ABORT(arg0)
+#define    TRACE_POSTGRESQL_TRANSACTION_ABORT_ENABLED() (0)
+
+#define    TRACE_POSTGRESQL_LWLOCK_STARTWAIT(arg0, arg1)
+#define    TRACE_POSTGRESQL_LWLOCK_STARTWAIT_ENABLED() (0)
+
+#define    TRACE_POSTGRESQL_LWLOCK_RELEASE(arg0)
+#define    TRACE_POSTGRESQL_LWLOCK_RELEASE_ENABLED() (0)
+
+#define    TRACE_POSTGRESQL_LWLOCK_ENDWAIT(arg0, arg1)
+#define    TRACE_POSTGRESQL_LWLOCK_ENDWAIT_ENABLED() (0)
+
+#define    TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(arg0, arg1)
+#define    TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL_ENABLED() (0)
+
+#define    TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE(arg0, arg1)
+#define    TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_ENABLED() (0)
+
+#define    TRACE_POSTGRESQL_LWLOCK_ACQUIRE(arg0, arg1)
+#define    TRACE_POSTGRESQL_LWLOCK_ACQUIRE_ENABLED() (0)
+
+#define    TRACE_POSTGRESQL_LOCK_STARTWAIT(arg0, arg1)
+#define    TRACE_POSTGRESQL_LOCK_STARTWAIT_ENABLED() (0)
+
+#define    TRACE_POSTGRESQL_LOCK_ENDWAIT(arg0, arg1)
+#define    TRACE_POSTGRESQL_LOCK_ENDWAIT_ENABLED() (0)

-#define PG_TRACE(name)
-#define PG_TRACE1(name, arg1)
-#define PG_TRACE2(name, arg1, arg2)
-#define PG_TRACE3(name, arg1, arg2, arg3)
-#define PG_TRACE4(name, arg1, arg2, arg3, arg4)
-#define PG_TRACE5(name, arg1, arg2, arg3, arg4, arg5)
 #endif   /* not ENABLE_DTRACE */

 #endif   /* PG_TRACE_H */

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@postgresql.org)
> To make changes to your subscription:
> http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.org&extra=pgsql-patches

--
  Bruce Momjian  <bruce@momjian.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@gmx.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.