Re: [HACKERS] dtrace probes

[Jesper Pedersen]

On 04/20/2017 10:30 AM, Jesper Pedersen wrote:

I think this fix is harmless and has some value in terms of
consistency.  One minor suggestion is that you should leave a space
after typecasting.

- TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
+ TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), (int)mode);

There should be a space like "(int) mode".




v2 attached.



I managed to attach the same patch again, so here is v3.

Best regards,
 Jesper


From 0d964df84950ca90c08ed6dd77a575d4b70ea7db Mon Sep 17 00:00:00 2001
From: jesperpedersen 
Date: Tue, 18 Apr 2017 11:44:18 -0400
Subject: [PATCH] Explicit cast LWLockMode, which an enum, to int in order to
 match the dtrace definition of the lwlock methods. Thereby all call sites
 will have the same definition instead of a mix between signed and unsigned.

Author: Jesper Pedersen 
---
 src/backend/storage/lmgr/lwlock.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/backend/storage/lmgr/lwlock.c 
b/src/backend/storage/lmgr/lwlock.c
index 3e13394..c551be2 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -1226,7 +1226,7 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
 #endif
 
LWLockReportWaitStart(lock);
-   TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode);
+   TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), (int) mode);
 
for (;;)
{
@@ -1248,7 +1248,7 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
}
 #endif
 
-   TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
+   TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), (int) mode);
LWLockReportWaitEnd();
 
LOG_LWDEBUG("LWLockAcquire", lock, "awakened");
@@ -1257,7 +1257,7 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
result = false;
}
 
-   TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(lock), mode);
+   TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(lock), (int) mode);
 
/* Add lock to list of locks held by this backend */
held_lwlocks[num_held_lwlocks].lock = lock;
@@ -1308,14 +1308,14 @@ LWLockConditionalAcquire(LWLock *lock, LWLockMode mode)
RESUME_INTERRUPTS();
 
LOG_LWDEBUG("LWLockConditionalAcquire", lock, "failed");
-   TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(T_NAME(lock), mode);
+   TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(T_NAME(lock), (int) 
mode);
}
else
{
/* Add lock to list of locks held by this backend */
held_lwlocks[num_held_lwlocks].lock = lock;
held_lwlocks[num_held_lwlocks++].mode = mode;
-   TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE(T_NAME(lock), mode);
+   TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE(T_NAME(lock), (int) mode);
}
return !mustwait;
 }
@@ -1387,7 +1387,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
 #endif
 
LWLockReportWaitStart(lock);
-   TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode);
+   TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), (int) 
mode);
 
for (;;)
{
@@ -1405,7 +1405,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
Assert(nwaiters < MAX_BACKENDS);
}
 #endif
-   TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
+   TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), (int) 
mode);
LWLockReportWaitEnd();
 
LOG_LWDEBUG("LWLockAcquireOrWait", lock, "awakened");
@@ -1435,7 +1435,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
/* Failed to get lock, so release interrupt holdoff */
RESUME_INTERRUPTS();
LOG_LWDEBUG("LWLockAcquireOrWait", lock, "failed");
-   TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT_FAIL(T_NAME(lock), 
mode);
+   TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT_FAIL(T_NAME(lock), 
(int) mode);
}
else
{
@@ -1443,7 +1443,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
/* Add lock to list of locks held by this backend */
held_lwlocks[num_held_lwlocks].lock = lock;
held_lwlocks[num_held_lwlocks++].mode = mode;
-   TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT(T_NAME(lock), mode);
+   TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT(T_NAME(lock), (int) 
mode);
}
 
return !mustwait;
-- 
2.7.4


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] dtrace probes

[Jesper Pedersen]

Hi,

On 04/20/2017 09:24 AM, Amit Kapila wrote:

The lwlock dtrace probes define LWLockMode as int, and the
TRACE_POSTGRESQL_LWLOCK methods are called using both a variable and
constant definition.

This leads to a mix of argument definitions depending on the call site, as
seen in probes.txt file.

A fix is to explicit cast 'mode' to int such that all call sites will use
the

 argument #2 4 signed   bytes

definition. Attached patch does this.



I think this fix is harmless and has some value in terms of
consistency.  One minor suggestion is that you should leave a space
after typecasting.

- TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
+ TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), (int)mode);

There should be a space like "(int) mode".




v2 attached.


I have verified all dtraces probes for their type, and only the lock__
methods doesn't aligned with its actual types.



Do you see any problem with that?



Not really, but it would be more clear what the value space of each of 
the parameters were.




Depending on the feedback I can add this patch to the open item list in
order to fix it for PostgreSQL 10.



Is there any commit in PG-10 which has caused this behavior?  If not,
then I don't think it should be added to open items of PG-10.




It is really a bug fix, so it could even be back patched.

Thanks for the feedback !

Best regards,
 Jesper


>From 7175dc8e05ff703229bd6cab6b254587ffc076c8 Mon Sep 17 00:00:00 2001
From: jesperpedersen 
Date: Tue, 18 Apr 2017 11:44:18 -0400
Subject: [PATCH] Explicit cast LWLockMode, which an enum, to int in order to
 match the dtrace definition of the lwlock methods. Thereby all call sites
 will have the same definition instead of a mix between signed and unsigned.

Author: Jesper Pedersen 
---
 src/backend/storage/lmgr/lwlock.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 3e13394..abf5fbb 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -1226,7 +1226,7 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
 #endif
 
 		LWLockReportWaitStart(lock);
-		TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode);
+		TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), (int)mode);
 
 		for (;;)
 		{
@@ -1248,7 +1248,7 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
 		}
 #endif
 
-		TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
+		TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), (int)mode);
 		LWLockReportWaitEnd();
 
 		LOG_LWDEBUG("LWLockAcquire", lock, "awakened");
@@ -1257,7 +1257,7 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
 		result = false;
 	}
 
-	TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(lock), mode);
+	TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(lock), (int)mode);
 
 	/* Add lock to list of locks held by this backend */
 	held_lwlocks[num_held_lwlocks].lock = lock;
@@ -1308,14 +1308,14 @@ LWLockConditionalAcquire(LWLock *lock, LWLockMode mode)
 		RESUME_INTERRUPTS();
 
 		LOG_LWDEBUG("LWLockConditionalAcquire", lock, "failed");
-		TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(T_NAME(lock), mode);
+		TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(T_NAME(lock), (int)mode);
 	}
 	else
 	{
 		/* Add lock to list of locks held by this backend */
 		held_lwlocks[num_held_lwlocks].lock = lock;
 		held_lwlocks[num_held_lwlocks++].mode = mode;
-		TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE(T_NAME(lock), mode);
+		TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE(T_NAME(lock), (int)mode);
 	}
 	return !mustwait;
 }
@@ -1387,7 +1387,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
 #endif
 
 			LWLockReportWaitStart(lock);
-			TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode);
+			TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), (int)mode);
 
 			for (;;)
 			{
@@ -1405,7 +1405,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
 Assert(nwaiters < MAX_BACKENDS);
 			}
 #endif
-			TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
+			TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), (int)mode);
 			LWLockReportWaitEnd();
 
 			LOG_LWDEBUG("LWLockAcquireOrWait", lock, "awakened");
@@ -1435,7 +1435,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
 		/* Failed to get lock, so release interrupt holdoff */
 		RESUME_INTERRUPTS();
 		LOG_LWDEBUG("LWLockAcquireOrWait", lock, "failed");
-		TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT_FAIL(T_NAME(lock), mode);
+		TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT_FAIL(T_NAME(lock), (int)mode);
 	}
 	else
 	{
@@ -1443,7 +1443,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
 		/* Add lock to list of locks held by this backend */
 		held_lwlocks[num_held_lwlocks].lock = lock;
 		held_lwlocks[num_held_lwlocks++].mode = mode;
-		TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT(T_NAME(lock), mode);
+		TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT(T_NAME(lock), (int)mode);
 	}
 
 	return !mustwait;
-- 
2.7.4


-- 
Sent via pgsql-hackers 

Re: [HACKERS] dtrace probes

[Amit Kapila]

On Tue, Apr 18, 2017 at 9:38 PM, Jesper Pedersen
 wrote:
> Hi,
>
> The lwlock dtrace probes define LWLockMode as int, and the
> TRACE_POSTGRESQL_LWLOCK methods are called using both a variable and
> constant definition.
>
> This leads to a mix of argument definitions depending on the call site, as
> seen in probes.txt file.
>
> A fix is to explicit cast 'mode' to int such that all call sites will use
> the
>
>  argument #2 4 signed   bytes
>
> definition. Attached patch does this.
>

I think this fix is harmless and has some value in terms of
consistency.  One minor suggestion is that you should leave a space
after typecasting.

- TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
+ TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), (int)mode);

There should be a space like "(int) mode".


> I have verified all dtraces probes for their type, and only the lock__
> methods doesn't aligned with its actual types.
>

Do you see any problem with that?

>
> Depending on the feedback I can add this patch to the open item list in
> order to fix it for PostgreSQL 10.
>

Is there any commit in PG-10 which has caused this behavior?  If not,
then I don't think it should be added to open items of PG-10.



-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] dtrace probes

[Jesper Pedersen]

Hi,

The lwlock dtrace probes define LWLockMode as int, and the 
TRACE_POSTGRESQL_LWLOCK methods are called using both a variable and 
constant definition.


This leads to a mix of argument definitions depending on the call site, 
as seen in probes.txt file.


A fix is to explicit cast 'mode' to int such that all call sites will 
use the


 argument #2 4 signed   bytes

definition. Attached patch does this.

I have verified all dtraces probes for their type, and only the lock__ 
methods doesn't aligned with its actual types. However, that would 
require a change to probes.d, and therefore PostgreSQL 11 material.


Depending on the feedback I can add this patch to the open item list in 
order to fix it for PostgreSQL 10.


Best regards,
 Jesper
>From d6f5c119c057c7ff8c84f88bb4122a1ca245a7d4 Mon Sep 17 00:00:00 2001
From: jesperpedersen 
Date: Tue, 18 Apr 2017 11:44:18 -0400
Subject: [PATCH] Explicit cast LWLockMode, which an enum, to int in order to
 match the dtrace definition of the lwlock methods. Thereby all call sites
 will have the same definition instead of a mix between signed and unsigned.

Author: Jesper Pedersen 
---
 src/backend/storage/lmgr/lwlock.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 3e13394..abf5fbb 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -1226,7 +1226,7 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
 #endif
 
 		LWLockReportWaitStart(lock);
-		TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode);
+		TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), (int)mode);
 
 		for (;;)
 		{
@@ -1248,7 +1248,7 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
 		}
 #endif
 
-		TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
+		TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), (int)mode);
 		LWLockReportWaitEnd();
 
 		LOG_LWDEBUG("LWLockAcquire", lock, "awakened");
@@ -1257,7 +1257,7 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
 		result = false;
 	}
 
-	TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(lock), mode);
+	TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(lock), (int)mode);
 
 	/* Add lock to list of locks held by this backend */
 	held_lwlocks[num_held_lwlocks].lock = lock;
@@ -1308,14 +1308,14 @@ LWLockConditionalAcquire(LWLock *lock, LWLockMode mode)
 		RESUME_INTERRUPTS();
 
 		LOG_LWDEBUG("LWLockConditionalAcquire", lock, "failed");
-		TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(T_NAME(lock), mode);
+		TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(T_NAME(lock), (int)mode);
 	}
 	else
 	{
 		/* Add lock to list of locks held by this backend */
 		held_lwlocks[num_held_lwlocks].lock = lock;
 		held_lwlocks[num_held_lwlocks++].mode = mode;
-		TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE(T_NAME(lock), mode);
+		TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE(T_NAME(lock), (int)mode);
 	}
 	return !mustwait;
 }
@@ -1387,7 +1387,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
 #endif
 
 			LWLockReportWaitStart(lock);
-			TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode);
+			TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), (int)mode);
 
 			for (;;)
 			{
@@ -1405,7 +1405,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
 Assert(nwaiters < MAX_BACKENDS);
 			}
 #endif
-			TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
+			TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), (int)mode);
 			LWLockReportWaitEnd();
 
 			LOG_LWDEBUG("LWLockAcquireOrWait", lock, "awakened");
@@ -1435,7 +1435,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
 		/* Failed to get lock, so release interrupt holdoff */
 		RESUME_INTERRUPTS();
 		LOG_LWDEBUG("LWLockAcquireOrWait", lock, "failed");
-		TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT_FAIL(T_NAME(lock), mode);
+		TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT_FAIL(T_NAME(lock), (int)mode);
 	}
 	else
 	{
@@ -1443,7 +1443,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
 		/* Add lock to list of locks held by this backend */
 		held_lwlocks[num_held_lwlocks].lock = lock;
 		held_lwlocks[num_held_lwlocks++].mode = mode;
-		TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT(T_NAME(lock), mode);
+		TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT(T_NAME(lock), (int)mode);
 	}
 
 	return !mustwait;
-- 
2.7.4

/usr/local/bin/postgres postgresql:clog__checkpoint__start [sema 0xe2351a]
  location #1 0x4fc706
argument #1 1 signed   bytes @ 0
  location #2 0x4fc72d
argument #1 1 signed   bytes @ 1
/usr/local/bin/postgres postgresql:clog__checkpoint__done [sema 0xe2351c]
  location #1 0x4fc725
argument #1 1 signed   bytes @ 0
  location #2 0x4fc74c
argument #1 1 signed   bytes @ 1
/usr/local/bin/postgres postgresql:multixact__checkpoint__start [sema 0xe23522]
  location #1 0x500d07
argument #1 1 signed   bytes @ 0
  location #2 0x500dbc
argument #1 1 signed   bytes @ 1
/usr/local/bin/postgres postgresql:multixact__checkpoint__done [sema 0xe23524]
  

Re: [HACKERS] Dtrace probes documentation

[Frank Ch. Eigler]

Tom Lane t...@sss.pgh.pa.us writes:

 [...]
 See http://blog.endpoint.com/2009/05/postgresql-with-systemtap.html for
 details. Perhaps it's worth noting in the documentation that SystemTap users
 will need to use the double-underscore version?

 I think a better solution is to persuade the Systemtap guys that they
 ought to accept the single-hyphen spelling.  [...]

Will do: http://sourceware.org/PR10225.

- FChE

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Dtrace probes documentation

[Tom Lane]

Joshua Tolley eggyk...@gmail.com writes:
 On Thu, May 28, 2009 at 06:28:14PM -0400, Tom Lane wrote:
 Read 26.4.3 and .4.  I don't know why they have this bizarre set of
 conventions, but the single-hyphen version is the spelling
 most visible to end users.

 I thought it might be something like that. I've been playing with SystemTap,
 and found that only the double-underscore version works for ... well, 
 anything.
 See http://blog.endpoint.com/2009/05/postgresql-with-systemtap.html for
 details. Perhaps it's worth noting in the documentation that SystemTap users
 will need to use the double-underscore version?

I think a better solution is to persuade the Systemtap guys that they
ought to accept the single-hyphen spelling.  I've put in a request for
that, we'll see what they think ...

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Dtrace probes documentation

[Joshua Tolley]

The dtrace probes documentation [1] spells each probe name with dashes
(transaction-start, transaction-commit, etc.). Yet as far as I can see,
dtrace only works if you spell the probe names with double underscores
(transaction__start, transaction__commit, etc.). Why the discrepancy?
Obvious patch attached, in case this needs to be changed.

- Josh / eggyknap

1: http://www.postgresql.org/docs/8.4/static/dynamic-trace.html

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index ea8b017..672a1fe 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1070,95 +1070,95 @@ SELECT pg_stat_get_backend_pid(s.backendid) AS procpid,
tbody
 
 row
- entrytransaction-start/entry
+ entrytransaction__start/entry
  entry(LocalTransactionId)/entry
  entryProbe that fires at the start of a new transaction.
   arg0 is the transaction id./entry
 /row
 row
- entrytransaction-commit/entry
+ entrytransaction__commit/entry
  entry(LocalTransactionId)/entry
  entryProbe that fires when a transaction completes successfully.
   arg0 is the transaction id./entry
 /row
 row
- entrytransaction-abort/entry
+ entrytransaction__abort/entry
  entry(LocalTransactionId)/entry
  entryProbe that fires when a transaction completes unsuccessfully.
   arg0 is the transaction id./entry
 /row
 row
- entryquery-start/entry
+ entryquery__start/entry
  entry(const char *)/entry
  entryProbe that fires when the processing of a query is started.
   arg0 is the query string./entry
 /row
 row
- entryquery-done/entry
+ entryquery__done/entry
  entry(const char *)/entry
  entryProbe that fires when the processing of a query is complete.
   arg0 is the query string./entry
 /row
 row
- entryquery-parse-start/entry
+ entryquery__parse__start/entry
  entry(const char *)/entry
  entryProbe that fires when the parsing of a query is started.
   arg0 is the query string./entry
 /row
 row
- entryquery-parse-done/entry
+ entryquery__parse__done/entry
  entry(const char *)/entry
  entryProbe that fires when the parsing of a query is complete.
   arg0 is the query string./entry
 /row
 row
- entryquery-rewrite-start/entry
+ entryquery__rewrite__start/entry
  entry(const char *)/entry
  entryProbe that fires when the rewriting of a query is started.
   arg0 is the query string./entry
 /row
 row
- entryquery-rewrite-done/entry
+ entryquery__rewrite__done/entry
  entry(const char *)/entry
  entryProbe that fires when the rewriting of a query is complete.
   arg0 is the query string./entry
 /row
 row
- entryquery-plan-start/entry
+ entryquery__plan__start/entry
  entry()/entry
  entryProbe that fires when the planning of a query is started./entry
 /row
 row
- entryquery-plan-done/entry
+ entryquery__plan__done/entry
  entry()/entry
  entryProbe that fires when the planning of a query is complete./entry
 /row
 row
- entryquery-execute-start/entry
+ entryquery__execute__start/entry
  entry()/entry
  entryProbe that fires when the execution of a query is started./entry
 /row
 row
- entryquery-execute-done/entry
+ entryquery__execute__done/entry
  entry()/entry
  entryProbe that fires when the execution of a query is complete./entry
 /row
 row
- entrystatement-status/entry
+ entrystatement__status/entry
  entry(const char *)/entry
  entryProbe that fires anytime the server process updates its
   structnamepg_stat_activity/.structfieldcurrent_query/ status.
   arg0 is the new status string./entry
 /row
 row
- entrycheckpoint-start/entry
+ entrycheckpoint__start/entry
  entry(int)/entry
  entryProbe that fires when a checkpoint is started.
   arg0 holds the bitwise flags used to distinguish different checkpoint
   types, such as shutdown, immediate or force./entry
 /row
 row
- entrycheckpoint-done/entry
+ entrycheckpoint__done/entry
  entry(int, int, int, int, int)/entry
  entryProbe that fires when a checkpoint is complete.
   (The probes listed next fire in sequence during checkpoint processing.)
@@ -1167,20 +1167,20 @@ SELECT pg_stat_get_backend_pid(s.backendid) AS procpid,
   removed and recycled respectively./entry
 /row
 row
- entryclog-checkpoint-start/entry
+ entryclog__checkpoint__start/entry
  entry(bool)/entry
  entryProbe that fires when the CLOG portion of a checkpoint is started.
   arg0 is true for normal checkpoint, false for shutdown
   checkpoint./entry
 /row
 row
- entryclog-checkpoint-done/entry
+ entryclog__checkpoint__done/entry
  entry(bool)/entry
  entryProbe that fires when the CLOG portion of a checkpoint is
-  

Re: [HACKERS] Dtrace probes documentation

[Tom Lane]

Joshua Tolley eggyk...@gmail.com writes:
 The dtrace probes documentation [1] spells each probe name with dashes
 (transaction-start, transaction-commit, etc.). Yet as far as I can see,
 dtrace only works if you spell the probe names with double underscores
 (transaction__start, transaction__commit, etc.). Why the discrepancy?

Read 26.4.3 and .4.  I don't know why they have this bizarre set of
conventions, but the single-hyphen version is the spelling
most visible to end users.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Dtrace probes documentation

[Joshua Tolley]

On Thu, May 28, 2009 at 06:28:14PM -0400, Tom Lane wrote:
 Joshua Tolley eggyk...@gmail.com writes:
  The dtrace probes documentation [1] spells each probe name with dashes
  (transaction-start, transaction-commit, etc.). Yet as far as I can see,
  dtrace only works if you spell the probe names with double underscores
  (transaction__start, transaction__commit, etc.). Why the discrepancy?
 
 Read 26.4.3 and .4.  I don't know why they have this bizarre set of
 conventions, but the single-hyphen version is the spelling
 most visible to end users.

I thought it might be something like that. I've been playing with SystemTap,
and found that only the double-underscore version works for ... well, anything.
See http://blog.endpoint.com/2009/05/postgresql-with-systemtap.html for
details. Perhaps it's worth noting in the documentation that SystemTap users
will need to use the double-underscore version?

- Josh / eggyknap


signature.asc
Description: Digital signature

Re: [HACKERS] DTrace probes broken in HEAD on Solaris?

[Tom Lane]

Robert Lor robert@sun.com writes:
 Tom Lane wrote:
 [ complaining about disabled probes not being no-ops ]

 1) Only use if (foo_ENABLED()) test  for probes with expensive function 
 call/computation in arguments. This will keep the code clean, and we can 
 document this in the Definine New Probes section in the online doc.
 ...
 I prefer option 1 the most and 3 the least.

I got the same advice from the systemtap people, so we'll do it that
way.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes broken in HEAD on Solaris?

[Zdenek Kotala]

Dne 24.03.09 22:31, Robert Lor napsal(a):

I think the is-enabled test will address the issues you encountered. I 
see a few alternative to fixing this:


1) Only use if (foo_ENABLED()) test  for probes with expensive function 
call/computation in arguments. This will keep the code clean, and we can 
document this in the Definine New Probes section in the online doc.


2) Add the if(foo_ENABLED()) test to all probes manually and make this a 
requirement for all future probes. This makes the check explicit and 
avoid confusion.


3) Post-process probes.h so if(foo_ENABLED()) test is added to every 
probe. We're doing some post-processing now by pre-pending TRACE_ to the 
macros with a sed script. Personally, I don't like doing complex 
post-processing of output from another tool because the script can break 
if for some reason DTrace's output is changed.


I prefer option 1 the most and 3 the least.


I prefer also option 1. In many cases if(foo_ENABLED) has same or bigger 
 performance penalty like disabled probe itself.


Zdenek

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes broken in HEAD on Solaris?

[Robert Lor]

Tom Lane wrote:

Zdenek Kotala zdenek.kot...@sun.com writes:
  
Answer why it happens when probes are disabled is, that for user 
application there are piece of code which prepare DTrace probes 
arguments which will be passed into kernel DTrace modul. This code has 
performance penalty which depends on number of arguments.



The other thing I don't like is that this implementation exposes people
to any bugs that may exist in the probe arguments, *even when they don't
have any tracing turned on*.  (Again, we had two different instances of
that last week, so don't bother arguing that it doesn't happen.)
  


Yes, the above scenario can occur when compiling with DTrace (passing 
--enable-dtrace to configure) even when the probes are not enabled. It 
won't be an issue if you don't compile with DTrace though as the macros 
are converted to no-ops. Hopefully, any bugs in the probe arguments 
would be caught during testing ;-)  


Both of these considerations are strong arguments for not building
production installations with --enable-dtrace, just as we don't
encourage people to build for production with --enable-cassert.

But of course part of the argument for dtrace support is that people
would like to have such probing available in production installations.

What I've found out about this is that for each probe macro, DTrace
also defines a foo_ENABLED() macro that can be used like this:

if (foo_ENABLED())
foo(...);

I think what we should do about these considerations is fix our macro
definitions so that the if(ENABLED()) test is built into the macros.
I'm not sure what this will require ... probably some post-processing
of the probes.h file ... but if we don't do it we're going to keep
getting bit.
  
I was contemplating on using the is-enabled test, but when checking the 
arguments to all the probes, they were quite simple (except the 
relpath() call which is now removed).


I think the is-enabled test will address the issues you encountered. I 
see a few alternative to fixing this:


1) Only use if (foo_ENABLED()) test  for probes with expensive function 
call/computation in arguments. This will keep the code clean, and we can 
document this in the Definine New Probes section in the online doc.


2) Add the if(foo_ENABLED()) test to all probes manually and make this a 
requirement for all future probes. This makes the check explicit and 
avoid confusion.


3) Post-process probes.h so if(foo_ENABLED()) test is added to every 
probe. We're doing some post-processing now by pre-pending TRACE_ to the 
macros with a sed script. Personally, I don't like doing complex 
post-processing of output from another tool because the script can break 
if for some reason DTrace's output is changed.


I prefer option 1 the most and 3 the least.


-Robert



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes broken in HEAD on Solaris?

[Greg Stark]

On Tue, Mar 24, 2009 at 9:31 PM, Robert Lor robert@sun.com wrote:

 I think the is-enabled test will address the issues you encountered. I see a
 few alternative to fixing this:


Another option is to impose a policy that all arguments to probes must
be simple local variables -- no expressions.

I'm starting to think that would be the better option and more in tune
with the dtrace way. Introducing the _ENABLED check defeats the whole
performance aim of dtrace that when it's disabled it introduces no
overhead. But using the _ENABLED macro only sparsely risks missing
some expression somewhere which looks innocuous but isn't.

I wonder if there's a gcc extension that would let us check if a macro
parameter is a simple variable or expression. That would let us
enforce the polilcy strictly at build-time.

-- 
greg

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes broken in HEAD on Solaris?

[Martijn van Oosterhout]

On Tue, Mar 24, 2009 at 09:49:50PM +, Greg Stark wrote:
 I wonder if there's a gcc extension that would let us check if a macro
 parameter is a simple variable or expression. That would let us
 enforce the polilcy strictly at build-time.

Not really a GCC extension, but you could have the macro check that it
can take the address of the arguments, which amounts to almost the same
thing. It only doesn't work on constants.

Have a nice day,
-- 
Martijn van Oosterhout   klep...@svana.org   http://svana.org/kleptog/
 Please line up in a tree and maintain the heap invariant while 
 boarding. Thank you for flying nlogn airlines.


signature.asc
Description: Digital signature

Re: [HACKERS] DTrace probes broken in HEAD on Solaris?

[Greg Stark]

On Tue, Mar 24, 2009 at 10:12 PM, Martijn van Oosterhout
klep...@svana.org wrote:
 Not really a GCC extension, but you could have the macro check that it
 can take the address of the arguments, which amounts to almost the same
 thing. It only doesn't work on constants.

No, there are all kinds of complex expressions which are lvalues --
anything ending in a pointer dereference for example, which is
precisely the most likely kind of expression to introduce seg fault if
something is unexpectedly null
.

-- 
greg

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes broken in HEAD on Solaris?

[Martijn van Oosterhout]

On Tue, Mar 24, 2009 at 10:18:08PM +, Greg Stark wrote:
 On Tue, Mar 24, 2009 at 10:12 PM, Martijn van Oosterhout
 klep...@svana.org wrote:
  Not really a GCC extension, but you could have the macro check that it
  can take the address of the arguments, which amounts to almost the same
  thing. It only doesn't work on constants.
 
 No, there are all kinds of complex expressions which are lvalues --
 anything ending in a pointer dereference for example, which is
 precisely the most likely kind of expression to introduce seg fault if
 something is unexpectedly null

Sorry, I meant to say that the compiler could determine the address at
compile time, something like:

__builtin_constant_p( !(__x)) )

Have a nice day,
-- 
Martijn van Oosterhout   klep...@svana.org   http://svana.org/kleptog/
 Please line up in a tree and maintain the heap invariant while 
 boarding. Thank you for flying nlogn airlines.


signature.asc
Description: Digital signature

Re: [HACKERS] DTrace probes broken in HEAD on Solaris?

[Greg Stark]

On Tue, Mar 24, 2009 at 10:53 PM, Martijn van Oosterhout
klep...@svana.org wrote:
 Sorry, I meant to say that the compiler could determine the address at
 compile time, something like:

 __builtin_constant_p( !(__x)) )

Hm, that's a better idea.

How about something like

(__builtin_constant_p(__x) || __builtin_constant_p(!(__x)))

This would still pass expressions like foo[x] even if x might be
out-of-bounds or foo might be pointed to a freed object or something
like that though.




-- 
greg

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes broken in HEAD on Solaris?

[Tom Lane]

Greg Stark st...@enterprisedb.com writes:
 On Tue, Mar 24, 2009 at 9:31 PM, Robert Lor robert@sun.com wrote:
 I think the is-enabled test will address the issues you encountered. I see a
 few alternative to fixing this:

 Another option is to impose a policy that all arguments to probes must
 be simple local variables -- no expressions.

IOW, if you need to trace on an expression, you have to calculate it
whether or not ENABLE_DTRACE is even defined?  This doesn't seem to
me that it solves anything.  The cases that are interesting are where a
probe needs a value that otherwise wouldn't be needed.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes broken in HEAD on Solaris?

[Zdenek Kotala]

Dne 17.03.09 19:49, Tom Lane napsal(a):

Zdenek Kotala zdenek.kot...@sun.com writes:
Answer why it happens when probes are disabled is, that for user 
application there are piece of code which prepare DTrace probes 
arguments which will be passed into kernel DTrace modul. This code has 
performance penalty which depends on number of arguments.


More specifically, it seems that DTrace is designed so that it evaluates
all the arguments to a probe macro before it decides whether to actually
take the trap or not.


yeah, it is valid for USDT. I guess main reason for this is 
implementation is that DTrace does not have idea how compiler generates 
code and where he can find arguments. Only what it knows is how to call 
a probe and this call is noped.



This seems to me to be a pretty bad/surprising behavior; it's not the
way that our Assert macros work, for example.  There's a performance
issue, which would probably only be brutally obvious if you had an
expensive function in the arguments.  (Before you say no one would do
that, note the relpath() calls I was complaining about last week.)
But we've been muttering about dropping probes into some extremely
hot hot-spots, like spinlock acquisition, and even a few more
instructions to copy local variables could make a difference there.


yeah, spinlock is problem, but on other side the probes are only in the 
branch when spinlock is not free and sleep anyway. And LWLOCK_ACQUIRE 
probe has minimal impact if you compare it with rest of LWLockAcquire 
function.



The other thing I don't like is that this implementation exposes people
to any bugs that may exist in the probe arguments, *even when they don't
have any tracing turned on*.  (Again, we had two different instances of
that last week, so don't bother arguing that it doesn't happen.)


I don't accept your argument here. Better if it fails every time not 
only when you enable the probe. DTrace is designated to be safe on 
production machine. It is not good to shut down postgresql server in 
production...



Both of these considerations are strong arguments for not building
production installations with --enable-dtrace, just as we don't
encourage people to build for production with --enable-cassert.

But of course part of the argument for dtrace support is that people
would like to have such probing available in production installations.

What I've found out about this is that for each probe macro, DTrace
also defines a foo_ENABLED() macro that can be used like this:

if (foo_ENABLED())
foo(...);

I think what we should do about these considerations is fix our macro
definitions so that the if(ENABLED()) test is built into the macros.
I'm not sure what this will require ... probably some post-processing
of the probes.h file ... but if we don't do it we're going to keep
getting bit.


I like this idea to used if(foo_ENABLED), but maybe we should used it 
only when args evaluation is more complicated?


I'm not sure if it is good idea to modify macros definition.

See how macro definition look like:

#define TRACE_POSTGRESQL_LWLOCK_ACQUIRE(arg0, arg1) \
__dtrace_postgresql___lwlock__acquire(arg0, arg1)
#define TRACE_POSTGRESQL_LWLOCK_ACQUIRE_ENABLED() \
__dtraceenabled_postgresql___lwlock__acquire()

Maybe add something like this at the end of probe.h:

#define TRACE_POSTGRESQL_LWLOCK_ACQUIRE_COND(arg0, arg1) \
if( TRACE_POSTGRESQL_LWLOCK_ACQUIRE_ENABLED() ) \
TRACE_POSTGRESQL_LWLOCK_ACQUIRE(arg0, arg1)



Zdenek

-
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes broken in HEAD on Solaris?

[Zdenek Kotala]

Sorry for late answer, I was on vacation last week. I see that you 
already fix a problem.


Answer why it happens when probes are disabled is, that for user 
application there are piece of code which prepare DTrace probes 
arguments which will be passed into kernel DTrace modul. This code has 
performance penalty which depends on number of arguments.


See the code:

FlushBuffer+0x69:   call   +0x128c6 smgropen
FlushBuffer+0x6e:   addl   $0x10,%esp
FlushBuffer+0x71:   movl   %eax,0xc(%ebp)
FlushBuffer+0x74:   subl   $0x4,%esp

FlushBuffer+0x77:   movl   0xc(%ebp),%eax
FlushBuffer+0x7a:   pushl  0x8(%eax)
FlushBuffer+0x7d:   pushl  0x4(%eax)
FlushBuffer+0x80:   pushl  (%eax)
FlushBuffer+0x82:   nop
FlushBuffer+0x83:   nop
FlushBuffer+0x84:   nop
FlushBuffer+0x85:   nop
FlushBuffer+0x86:   nop
FlushBuffer+0x87:   addl   $0x10,%esp


nops reserve space for probe.


For better explanation see:

http://blogs.sun.com/ahl/entry/user_land_tracing_gets_better


Zdenek


Dne 13.03.09 15:15, Tom Lane napsal(a):

Most of the Solaris buildfarm members are unhappy this morning.
It looks like the ones that are busted have --enable-dtrace,
which points the finger at the dtrace probe changes I made yesterday:
http://archives.postgresql.org/pgsql-committers/2009-03/msg00079.php

Those changes work on Linux and OS X, so it's not clear what I did
wrong.  Can anyone with a Solaris machine poke into it, at least
to the extent of finding out where it's dumping core?

regards, tom lane




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes broken in HEAD on Solaris?

[Tom Lane]

Zdenek Kotala zdenek.kot...@sun.com writes:
 Answer why it happens when probes are disabled is, that for user 
 application there are piece of code which prepare DTrace probes 
 arguments which will be passed into kernel DTrace modul. This code has 
 performance penalty which depends on number of arguments.

More specifically, it seems that DTrace is designed so that it evaluates
all the arguments to a probe macro before it decides whether to actually
take the trap or not.

This seems to me to be a pretty bad/surprising behavior; it's not the
way that our Assert macros work, for example.  There's a performance
issue, which would probably only be brutally obvious if you had an
expensive function in the arguments.  (Before you say no one would do
that, note the relpath() calls I was complaining about last week.)
But we've been muttering about dropping probes into some extremely
hot hot-spots, like spinlock acquisition, and even a few more
instructions to copy local variables could make a difference there.

The other thing I don't like is that this implementation exposes people
to any bugs that may exist in the probe arguments, *even when they don't
have any tracing turned on*.  (Again, we had two different instances of
that last week, so don't bother arguing that it doesn't happen.)

Both of these considerations are strong arguments for not building
production installations with --enable-dtrace, just as we don't
encourage people to build for production with --enable-cassert.

But of course part of the argument for dtrace support is that people
would like to have such probing available in production installations.

What I've found out about this is that for each probe macro, DTrace
also defines a foo_ENABLED() macro that can be used like this:

if (foo_ENABLED())
foo(...);

I think what we should do about these considerations is fix our macro
definitions so that the if(ENABLED()) test is built into the macros.
I'm not sure what this will require ... probably some post-processing
of the probes.h file ... but if we don't do it we're going to keep
getting bit.

Comments?

regards, tom lane

-
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] DTrace probes broken in HEAD on Solaris?

[Tom Lane]

Most of the Solaris buildfarm members are unhappy this morning.
It looks like the ones that are busted have --enable-dtrace,
which points the finger at the dtrace probe changes I made yesterday:
http://archives.postgresql.org/pgsql-committers/2009-03/msg00079.php

Those changes work on Linux and OS X, so it's not clear what I did
wrong.  Can anyone with a Solaris machine poke into it, at least
to the extent of finding out where it's dumping core?

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes patch

[Bruce Momjian]

Thanks, applied.

---

Robert Lor wrote:
 Tom Lane wrote:
  Robert Lor robert@sun.com writes:

  Tom Lane wrote:
  
  I agree.  If the probe is meant to track only *some* WAL writes
  then it needs to be named something less generic than
  TRACE_POSTGRESQL_WAL_BUFFER_WRITE.
 

  How about change it to TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DIRTY similar to 
  TRACE_POSTGRESQL_BUFFER_WRITE_DIRTY for shared buffers?
  
 
  Works for me...
 
  
 Attached is the patch for the above name change.
 
 
 -Robert

 Index: src/backend/access/transam/xlog.c
 ===
 RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/xlog.c,v
 retrieving revision 1.324
 diff -u -3 -p -r1.324 xlog.c
 --- src/backend/access/transam/xlog.c 17 Dec 2008 01:39:03 -  1.324
 +++ src/backend/access/transam/xlog.c 22 Dec 2008 16:28:00 -
 @@ -1318,14 +1318,14 @@ AdvanceXLInsertBuffer(bool new_segment)
* Have to write buffers while holding insert 
 lock. This is
* not good, so only write as much as we 
 absolutely must.
*/
 - TRACE_POSTGRESQL_WAL_BUFFER_WRITE_START();
 + TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DIRTY_START();
   WriteRqst.Write = OldPageRqstPtr;
   WriteRqst.Flush.xlogid = 0;
   WriteRqst.Flush.xrecoff = 0;
   XLogWrite(WriteRqst, false, false);
   LWLockRelease(WALWriteLock);
   Insert-LogwrtResult = LogwrtResult;
 - TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DONE();
 + TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DIRTY_DONE();
   }
   }
   }
 Index: src/backend/utils/probes.d
 ===
 RCS file: /projects/cvsroot/pgsql/src/backend/utils/probes.d,v
 retrieving revision 1.4
 diff -u -3 -p -r1.4 probes.d
 --- src/backend/utils/probes.d17 Dec 2008 01:39:04 -  1.4
 +++ src/backend/utils/probes.d22 Dec 2008 16:28:01 -
 @@ -89,6 +89,6 @@ provider postgresql {
  
   probe xlog__insert(unsigned char, unsigned char);
   probe xlog__switch();
 - probe wal__buffer__write__start();
 - probe wal__buffer__write__done();
 + probe wal__buffer__write__dirty__start();
 + probe wal__buffer__write__dirty__done();
  };

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes patch

[Robert Lor]

Tom Lane wrote:

Robert Lor robert@sun.com writes:
  

Tom Lane wrote:


I agree.  If the probe is meant to track only *some* WAL writes
then it needs to be named something less generic than
TRACE_POSTGRESQL_WAL_BUFFER_WRITE.

  

How about change it to TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DIRTY similar to 
TRACE_POSTGRESQL_BUFFER_WRITE_DIRTY for shared buffers?



Works for me...



Attached is the patch for the above name change.


-Robert
Index: src/backend/access/transam/xlog.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/xlog.c,v
retrieving revision 1.324
diff -u -3 -p -r1.324 xlog.c
--- src/backend/access/transam/xlog.c   17 Dec 2008 01:39:03 -  1.324
+++ src/backend/access/transam/xlog.c   22 Dec 2008 16:28:00 -
@@ -1318,14 +1318,14 @@ AdvanceXLInsertBuffer(bool new_segment)
 * Have to write buffers while holding insert 
lock. This is
 * not good, so only write as much as we 
absolutely must.
 */
-   TRACE_POSTGRESQL_WAL_BUFFER_WRITE_START();
+   TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DIRTY_START();
WriteRqst.Write = OldPageRqstPtr;
WriteRqst.Flush.xlogid = 0;
WriteRqst.Flush.xrecoff = 0;
XLogWrite(WriteRqst, false, false);
LWLockRelease(WALWriteLock);
Insert-LogwrtResult = LogwrtResult;
-   TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DONE();
+   TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DIRTY_DONE();
}
}
}
Index: src/backend/utils/probes.d
===
RCS file: /projects/cvsroot/pgsql/src/backend/utils/probes.d,v
retrieving revision 1.4
diff -u -3 -p -r1.4 probes.d
--- src/backend/utils/probes.d  17 Dec 2008 01:39:04 -  1.4
+++ src/backend/utils/probes.d  22 Dec 2008 16:28:01 -
@@ -89,6 +89,6 @@ provider postgresql {
 
probe xlog__insert(unsigned char, unsigned char);
probe xlog__switch();
-   probe wal__buffer__write__start();
-   probe wal__buffer__write__done();
+   probe wal__buffer__write__dirty__start();
+   probe wal__buffer__write__dirty__done();
 };

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes patch

[Robert Lor]

Tom Lane wrote:

Fujii Masao masao.fu...@gmail.com writes:
  

I understood your intention. But, I think that its function name is somewhat
confusing.



I agree.  If the probe is meant to track only *some* WAL writes
then it needs to be named something less generic than
TRACE_POSTGRESQL_WAL_BUFFER_WRITE.

  

How about change it to TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DIRTY similar to 
TRACE_POSTGRESQL_BUFFER_WRITE_DIRTY for shared buffers?


-Robert


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes patch

[Tom Lane]

Robert Lor robert@sun.com writes:
 Tom Lane wrote:
 I agree.  If the probe is meant to track only *some* WAL writes
 then it needs to be named something less generic than
 TRACE_POSTGRESQL_WAL_BUFFER_WRITE.
 
 How about change it to TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DIRTY similar to 
 TRACE_POSTGRESQL_BUFFER_WRITE_DIRTY for shared buffers?

Works for me...

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes patch

[Tom Lane]

Fujii Masao masao.fu...@gmail.com writes:
 On Thu, Dec 18, 2008 at 4:49 AM, Robert Lor robert@sun.com wrote:
 My understanding is that we only want to track the XLogWrite when advancing
 to the next buffer page, and if there is unwritten data in the new buffer
 page, that indicates no more empty WAL buffer pages available, but I may be
 wrong. I did some tests by adjusting wal_buffers, and I could observe this
 behavior, more calls to XLogWrite with smaller wal_buffers.

 I understood your intention. But, I think that its function name is somewhat
 confusing.

I agree.  If the probe is meant to track only *some* WAL writes
then it needs to be named something less generic than
TRACE_POSTGRESQL_WAL_BUFFER_WRITE.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes patch

[Robert Lor]

Alvaro Herrera wrote:

But there are 5 callers of XLogWrite ... why aren't the other ones being
tracked too?

  


This probe originally came from Simon, so it can't possibly be wrong :-)

My understanding is that we only want to track the XLogWrite when 
advancing to the next buffer page, and if there is unwritten data in the 
new buffer page, that indicates no more empty WAL buffer pages 
available, but I may be wrong. I did some tests by adjusting 
wal_buffers, and I could observe this behavior, more calls to XLogWrite 
with smaller wal_buffers.


-Robert



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes patch

[Alvaro Herrera]

Robert Lor escribió:
 Fujii Masao wrote:
 Hi,

 On Wed, Dec 17, 2008 at 4:53 AM, Robert Lor robert@sun.com wrote:

 Why is TRACE_POSTGRESQL_WAL_BUFFER_WRITE_START/DONE called
 only in AdvanceXLInsertBuffer? We can trace only a part of WAL buffer write?
   
 The intention of these probes is to determine if wal_buffers is too  
 small by monitoring how frequent the server has to write out the buffers  
 along with the I/O time.

But there are 5 callers of XLogWrite ... why aren't the other ones being
tracked too?

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes patch

[Robert Lor]

Fujii Masao wrote:

Hi,

On Wed, Dec 17, 2008 at 4:53 AM, Robert Lor robert@sun.com wrote:
  

@@ -1313,12 +1318,14 @@ AdvanceXLInsertBuffer(bool new_segment)
* Have to write buffers while holding insert
lock. This is
* not good, so only write as much as we
absolutely must.
*/
+   TRACE_POSTGRESQL_WAL_BUFFER_WRITE_START();
   WriteRqst.Write = OldPageRqstPtr;
   WriteRqst.Flush.xlogid = 0;
   WriteRqst.Flush.xrecoff = 0;
   XLogWrite(WriteRqst, false, false);
   LWLockRelease(WALWriteLock);
   Insert-LogwrtResult = LogwrtResult;
+   TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DONE();



Why is TRACE_POSTGRESQL_WAL_BUFFER_WRITE_START/DONE called
only in AdvanceXLInsertBuffer? We can trace only a part of WAL buffer write?

  
The intention of these probes is to determine if wal_buffers is too 
small by monitoring how frequent the server has to write out the buffers 
along with the I/O time.


-Robert



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes patch

[Fujii Masao]

Hi,

On Thu, Dec 18, 2008 at 4:49 AM, Robert Lor robert@sun.com wrote:
 Alvaro Herrera wrote:

 But there are 5 callers of XLogWrite ... why aren't the other ones being
 tracked too?



 This probe originally came from Simon, so it can't possibly be wrong :-)

 My understanding is that we only want to track the XLogWrite when advancing
 to the next buffer page, and if there is unwritten data in the new buffer
 page, that indicates no more empty WAL buffer pages available, but I may be
 wrong. I did some tests by adjusting wal_buffers, and I could observe this
 behavior, more calls to XLogWrite with smaller wal_buffers.

I understood your intention. But, I think that its function name is somewhat
confusing.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes patch

[Robert Lor]

Peter Eisentraut wrote:

Robert Lor wrote:


The attached patch contains a couple of fixes in the existing probes 
and includes a few new ones.


- Fixed compilation errors on OS X for probes that use typedefs


Could you explain what these errors are about?  I don't see any errors 
on my machine.


In the current probes.d, the following probe definitions are commented 
out because they cause compilation errors on OS X.


* probe lock__wait__start(unsigned int, LOCKMODE);
* probe lock__wait__done(unsigned int, LOCKMODE);
* probe buffer__read__start(BlockNumber, Oid, Oid, Oid, bool);
* probe buffer__read__done(BlockNumber, Oid, Oid, Oid, bool, bool);

The problem was fixed by making the changes below.  probes.d is 
preprocessed with cpp and as such only  macros get expanded.


From:

typedef unsigned int LocalTransactionId;
typedef int LWLockId;
typedef int LWLockMode;
typedef int LOCKMODE;
typedef unsigned int BlockNumber;
typedef unsigned int Oid;
typedef int ForkNumber;


To:

#define LocalTransactionId unsigned int
#define LWLockId int
#define LWLockMode int
#define LOCKMODE int
#define BlockNumber unsigned int
#define Oid unsigned int
#define ForkNumber int





--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes patch

[Bruce Momjian]

Robert Lor wrote:
 Bruce Momjian wrote:
  Should I apply this or hold it for 8.5?
 
 

 I think it should go into 8.4 as it also fixes existing problems.

I am seeing the following error when linking the backend on a BSD machine:

./../src/port/libpgport_srv.a -lssl -lcrypto -lgetopt -ldl -lutil -lm -o
postgres
storage/buffer/bufmgr.o: In function `ReadBuffer_common':
storage/buffer/bufmgr.o(.text+0x4e2): undefined reference to
`TRACE_POSTGRESQL_BUFFER_READ_DONE'
storage/smgr/md.o: In function `mdread':
storage/smgr/md.o(.text+0x8a7): undefined reference to
`TRACE_POSTGRESQL_SMGR_MD_READ_DONE'
storage/smgr/md.o: In function `mdwrite':
storage/smgr/md.o(.text+0xab6): undefined reference to
`TRACE_POSTGRESQL_SMGR_MD_WRITE_DONE'
gmake[2]: *** [postgres] Error 1
gmake[2]: Leaving directory
`/usr/var/local/src/gen/pgsql/CURRENT/pgsql/src/backend'
gmake[1]: *** [all] Error 2
gmake[1]: Leaving directory
`/usr/var/local/src/gen/pgsql/CURRENT/pgsql/src'


-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes patch

[Alvaro Herrera]

Bruce Momjian wrote:

 I am seeing the following error when linking the backend on a BSD machine:
 
   ./../src/port/libpgport_srv.a -lssl -lcrypto -lgetopt -ldl -lutil -lm -o
   postgres
   storage/buffer/bufmgr.o: In function `ReadBuffer_common':
   storage/buffer/bufmgr.o(.text+0x4e2): undefined reference to
   `TRACE_POSTGRESQL_BUFFER_READ_DONE'
   storage/smgr/md.o: In function `mdread':
   storage/smgr/md.o(.text+0x8a7): undefined reference to

The reason is that Gen_dummy_probes.sed handles only up to 6 args, and
this function has 7.  Just add one more line to that file.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes patch

[Alvaro Herrera]

Robert Lor wrote:
 Peter Eisentraut wrote:
 Robert Lor wrote:

 The attached patch contains a couple of fixes in the existing probes  
 and includes a few new ones.

 - Fixed compilation errors on OS X for probes that use typedefs

 Could you explain what these errors are about?  I don't see any errors  
 on my machine.

 In the current probes.d, the following probe definitions are commented  
 out because they cause compilation errors on OS X.

Yeah, this was something we agreed to fix when we committed the previous
DTrace patch.  The current code was said to be a stopgap.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes patch

[Robert Lor]

Bruce Momjian wrote:

Should I apply this or hold it for 8.5?


  

I think it should go into 8.4 as it also fixes existing problems.

-Robert


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes patch

[Peter Eisentraut]

Robert Lor wrote:


The attached patch contains a couple of fixes in the existing probes and 
includes a few new ones.


- Fixed compilation errors on OS X for probes that use typedefs


Could you explain what these errors are about?  I don't see any errors 
on my machine.



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace probes patch

[Robert Lor]

Bruce Momjian wrote:


I am seeing the following error when linking the backend on a BSD machine:


  
The updated patch attached should fix this problem. My bad for 
overlooking this.


-Robert


Index: src/backend/access/transam/xlog.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/xlog.c,v
retrieving revision 1.322
diff -u -3 -p -r1.322 xlog.c
--- src/backend/access/transam/xlog.c   9 Nov 2008 17:51:15 -   1.322
+++ src/backend/access/transam/xlog.c   16 Dec 2008 19:46:08 -
@@ -48,6 +48,7 @@
 #include utils/builtins.h
 #include utils/guc.h
 #include utils/ps_status.h
+#include pg_trace.h
 
 
 /* File path names (all relative to $PGDATA) */
@@ -486,6 +487,8 @@ XLogInsert(RmgrId rmid, uint8 info, XLog
if (info  XLR_INFO_MASK)
elog(PANIC, invalid xlog info mask %02X, info);
 
+   TRACE_POSTGRESQL_XLOG_INSERT(rmid, info);
+
/*
 * In bootstrap mode, we don't actually log anything but XLOG resources;
 * return a phony record pointer.
@@ -914,6 +917,8 @@ begin:;
XLogwrtRqst FlushRqst;
XLogRecPtr  OldSegEnd;
 
+   TRACE_POSTGRESQL_XLOG_SWITCH();
+
LWLockAcquire(WALWriteLock, LW_EXCLUSIVE);
 
/*
@@ -1313,12 +1318,14 @@ AdvanceXLInsertBuffer(bool new_segment)
 * Have to write buffers while holding insert 
lock. This is
 * not good, so only write as much as we 
absolutely must.
 */
+   TRACE_POSTGRESQL_WAL_BUFFER_WRITE_START();
WriteRqst.Write = OldPageRqstPtr;
WriteRqst.Flush.xlogid = 0;
WriteRqst.Flush.xrecoff = 0;
XLogWrite(WriteRqst, false, false);
LWLockRelease(WALWriteLock);
Insert-LogwrtResult = LogwrtResult;
+   TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DONE();
}
}
}
@@ -5904,6 +5911,8 @@ CreateCheckPoint(int flags)
if (log_checkpoints)
LogCheckpointStart(flags);
 
+   TRACE_POSTGRESQL_CHECKPOINT_START(flags);
+
/*
 * Before flushing data, we must wait for any transactions that are
 * currently in their commit critical sections.  If an xact inserted its
@@ -6069,6 +6078,11 @@ CreateCheckPoint(int flags)
if (log_checkpoints)
LogCheckpointEnd();
 
+TRACE_POSTGRESQL_CHECKPOINT_DONE(CheckpointStats.ckpt_bufs_written,
+NBuffers, CheckpointStats.ckpt_segs_added,
+CheckpointStats.ckpt_segs_removed,
+CheckpointStats.ckpt_segs_recycled);
+
LWLockRelease(CheckpointLock);
 }
 
Index: src/backend/storage/buffer/bufmgr.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/storage/buffer/bufmgr.c,v
retrieving revision 1.242
diff -u -3 -p -r1.242 bufmgr.c
--- src/backend/storage/buffer/bufmgr.c 19 Nov 2008 10:34:52 -  1.242
+++ src/backend/storage/buffer/bufmgr.c 16 Dec 2008 19:46:11 -
@@ -203,8 +203,7 @@ ReadBuffer_common(SMgrRelation smgr, boo
if (isExtend)
blockNum = smgrnblocks(smgr, forkNum);
 
-   TRACE_POSTGRESQL_BUFFER_READ_START(blockNum, smgr-smgr_rnode.spcNode,
-   smgr-smgr_rnode.dbNode, smgr-smgr_rnode.relNode, isLocalBuf);
+   TRACE_POSTGRESQL_BUFFER_READ_START(forkNum, blockNum, 
smgr-smgr_rnode.spcNode, smgr-smgr_rnode.dbNode, smgr-smgr_rnode.relNode, 
isLocalBuf);
 
if (isLocalBuf)
{
@@ -253,7 +252,7 @@ ReadBuffer_common(SMgrRelation smgr, boo
if (VacuumCostActive)
VacuumCostBalance += VacuumCostPageHit;
 
-   TRACE_POSTGRESQL_BUFFER_READ_DONE(blockNum,
+   TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum,
smgr-smgr_rnode.spcNode,
smgr-smgr_rnode.dbNode,
smgr-smgr_rnode.relNode, isLocalBuf, found);
@@ -380,9 +379,9 @@ ReadBuffer_common(SMgrRelation smgr, boo
if (VacuumCostActive)
VacuumCostBalance += VacuumCostPageMiss;
 
-   TRACE_POSTGRESQL_BUFFER_READ_DONE(blockNum, smgr-smgr_rnode.spcNode,
-   smgr-smgr_rnode.dbNode, smgr-smgr_rnode.relNode,
-   isLocalBuf, found);
+   TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum,
+   smgr-smgr_rnode.spcNode, smgr-smgr_rnode.dbNode,
+   smgr-smgr_rnode.relNode, isLocalBuf, found);
 
return BufferDescriptorGetBuffer(bufHdr);
 }
@@ 

Re: [HACKERS] DTrace probes patch

[Bruce Momjian]

Thanks, applied.

---

Robert Lor wrote:
 Bruce Momjian wrote:
 
  I am seeing the following error when linking the backend on a BSD machine:
 
 

 The updated patch attached should fix this problem. My bad for 
 overlooking this.
 
 -Robert
 
 

 Index: src/backend/access/transam/xlog.c
 ===
 RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/xlog.c,v
 retrieving revision 1.322
 diff -u -3 -p -r1.322 xlog.c
 --- src/backend/access/transam/xlog.c 9 Nov 2008 17:51:15 -   1.322
 +++ src/backend/access/transam/xlog.c 16 Dec 2008 19:46:08 -
 @@ -48,6 +48,7 @@
  #include utils/builtins.h
  #include utils/guc.h
  #include utils/ps_status.h
 +#include pg_trace.h
  
  
  /* File path names (all relative to $PGDATA) */
 @@ -486,6 +487,8 @@ XLogInsert(RmgrId rmid, uint8 info, XLog
   if (info  XLR_INFO_MASK)
   elog(PANIC, invalid xlog info mask %02X, info);
  
 + TRACE_POSTGRESQL_XLOG_INSERT(rmid, info);
 +
   /*
* In bootstrap mode, we don't actually log anything but XLOG resources;
* return a phony record pointer.
 @@ -914,6 +917,8 @@ begin:;
   XLogwrtRqst FlushRqst;
   XLogRecPtr  OldSegEnd;
  
 + TRACE_POSTGRESQL_XLOG_SWITCH();
 +
   LWLockAcquire(WALWriteLock, LW_EXCLUSIVE);
  
   /*
 @@ -1313,12 +1318,14 @@ AdvanceXLInsertBuffer(bool new_segment)
* Have to write buffers while holding insert 
 lock. This is
* not good, so only write as much as we 
 absolutely must.
*/
 + TRACE_POSTGRESQL_WAL_BUFFER_WRITE_START();
   WriteRqst.Write = OldPageRqstPtr;
   WriteRqst.Flush.xlogid = 0;
   WriteRqst.Flush.xrecoff = 0;
   XLogWrite(WriteRqst, false, false);
   LWLockRelease(WALWriteLock);
   Insert-LogwrtResult = LogwrtResult;
 + TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DONE();
   }
   }
   }
 @@ -5904,6 +5911,8 @@ CreateCheckPoint(int flags)
   if (log_checkpoints)
   LogCheckpointStart(flags);
  
 + TRACE_POSTGRESQL_CHECKPOINT_START(flags);
 +
   /*
* Before flushing data, we must wait for any transactions that are
* currently in their commit critical sections.  If an xact inserted its
 @@ -6069,6 +6078,11 @@ CreateCheckPoint(int flags)
   if (log_checkpoints)
   LogCheckpointEnd();
  
 +TRACE_POSTGRESQL_CHECKPOINT_DONE(CheckpointStats.ckpt_bufs_written,
 +NBuffers, CheckpointStats.ckpt_segs_added,
 +CheckpointStats.ckpt_segs_removed,
 +CheckpointStats.ckpt_segs_recycled);
 +
   LWLockRelease(CheckpointLock);
  }
  
 Index: src/backend/storage/buffer/bufmgr.c
 ===
 RCS file: /projects/cvsroot/pgsql/src/backend/storage/buffer/bufmgr.c,v
 retrieving revision 1.242
 diff -u -3 -p -r1.242 bufmgr.c
 --- src/backend/storage/buffer/bufmgr.c   19 Nov 2008 10:34:52 -  
 1.242
 +++ src/backend/storage/buffer/bufmgr.c   16 Dec 2008 19:46:11 -
 @@ -203,8 +203,7 @@ ReadBuffer_common(SMgrRelation smgr, boo
   if (isExtend)
   blockNum = smgrnblocks(smgr, forkNum);
  
 - TRACE_POSTGRESQL_BUFFER_READ_START(blockNum, smgr-smgr_rnode.spcNode,
 - smgr-smgr_rnode.dbNode, smgr-smgr_rnode.relNode, isLocalBuf);
 + TRACE_POSTGRESQL_BUFFER_READ_START(forkNum, blockNum, 
 smgr-smgr_rnode.spcNode, smgr-smgr_rnode.dbNode, smgr-smgr_rnode.relNode, 
 isLocalBuf);
  
   if (isLocalBuf)
   {
 @@ -253,7 +252,7 @@ ReadBuffer_common(SMgrRelation smgr, boo
   if (VacuumCostActive)
   VacuumCostBalance += VacuumCostPageHit;
  
 - TRACE_POSTGRESQL_BUFFER_READ_DONE(blockNum,
 + TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum,
   smgr-smgr_rnode.spcNode,
   smgr-smgr_rnode.dbNode,
   smgr-smgr_rnode.relNode, isLocalBuf, found);
 @@ -380,9 +379,9 @@ ReadBuffer_common(SMgrRelation smgr, boo
   if (VacuumCostActive)
   VacuumCostBalance += VacuumCostPageMiss;
  
 - TRACE_POSTGRESQL_BUFFER_READ_DONE(blockNum, smgr-smgr_rnode.spcNode,
 - smgr-smgr_rnode.dbNode, smgr-smgr_rnode.relNode,
 - isLocalBuf, found);
 + TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum,
 + smgr-smgr_rnode.spcNode, 

Re: [HACKERS] DTrace probes patch

[Fujii Masao]

Hi,

On Wed, Dec 17, 2008 at 4:53 AM, Robert Lor robert@sun.com wrote:
 @@ -1313,12 +1318,14 @@ AdvanceXLInsertBuffer(bool new_segment)
 * Have to write buffers while holding insert
 lock. This is
 * not good, so only write as much as we
 absolutely must.
 */
 +   TRACE_POSTGRESQL_WAL_BUFFER_WRITE_START();
WriteRqst.Write = OldPageRqstPtr;
WriteRqst.Flush.xlogid = 0;
WriteRqst.Flush.xrecoff = 0;
XLogWrite(WriteRqst, false, false);
LWLockRelease(WALWriteLock);
Insert-LogwrtResult = LogwrtResult;
 +   TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DONE();

Why is TRACE_POSTGRESQL_WAL_BUFFER_WRITE_START/DONE called
only in AdvanceXLInsertBuffer? We can trace only a part of WAL buffer write?

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] DTrace probes patch

[Robert Lor]

The attached patch contains a couple of fixes in the existing probes and 
includes a few new ones.


- Fixed compilation errors on OS X for probes that use typedefs
- Fixed a number of probes to pass ForkNumber per the relation forks patch
- The new probes are those that were taken out from the previous 
submitted patch and required simple fixes. Will submit the other probes 
that may require more discussion in a separate patch.



-Robert




Index: src/backend/access/transam/xlog.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/xlog.c,v
retrieving revision 1.322
diff -u -3 -p -r1.322 xlog.c
--- src/backend/access/transam/xlog.c   9 Nov 2008 17:51:15 -   1.322
+++ src/backend/access/transam/xlog.c   15 Dec 2008 05:12:41 -
@@ -48,6 +48,7 @@
 #include utils/builtins.h
 #include utils/guc.h
 #include utils/ps_status.h
+#include pg_trace.h
 
 
 /* File path names (all relative to $PGDATA) */
@@ -486,6 +487,8 @@ XLogInsert(RmgrId rmid, uint8 info, XLog
if (info  XLR_INFO_MASK)
elog(PANIC, invalid xlog info mask %02X, info);
 
+   TRACE_POSTGRESQL_XLOG_INSERT(rmid, info);
+
/*
 * In bootstrap mode, we don't actually log anything but XLOG resources;
 * return a phony record pointer.
@@ -914,6 +917,8 @@ begin:;
XLogwrtRqst FlushRqst;
XLogRecPtr  OldSegEnd;
 
+   TRACE_POSTGRESQL_XLOG_SWITCH();
+
LWLockAcquire(WALWriteLock, LW_EXCLUSIVE);
 
/*
@@ -1313,12 +1318,14 @@ AdvanceXLInsertBuffer(bool new_segment)
 * Have to write buffers while holding insert 
lock. This is
 * not good, so only write as much as we 
absolutely must.
 */
+   TRACE_POSTGRESQL_WAL_BUFFER_WRITE_START();
WriteRqst.Write = OldPageRqstPtr;
WriteRqst.Flush.xlogid = 0;
WriteRqst.Flush.xrecoff = 0;
XLogWrite(WriteRqst, false, false);
LWLockRelease(WALWriteLock);
Insert-LogwrtResult = LogwrtResult;
+   TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DONE();
}
}
}
@@ -5904,6 +5911,8 @@ CreateCheckPoint(int flags)
if (log_checkpoints)
LogCheckpointStart(flags);
 
+   TRACE_POSTGRESQL_CHECKPOINT_START(flags);
+
/*
 * Before flushing data, we must wait for any transactions that are
 * currently in their commit critical sections.  If an xact inserted its
@@ -6069,6 +6078,11 @@ CreateCheckPoint(int flags)
if (log_checkpoints)
LogCheckpointEnd();
 
+TRACE_POSTGRESQL_CHECKPOINT_DONE(CheckpointStats.ckpt_bufs_written,
+NBuffers, CheckpointStats.ckpt_segs_added,
+CheckpointStats.ckpt_segs_removed,
+CheckpointStats.ckpt_segs_recycled);
+
LWLockRelease(CheckpointLock);
 }
 
Index: src/backend/storage/buffer/bufmgr.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/storage/buffer/bufmgr.c,v
retrieving revision 1.242
diff -u -3 -p -r1.242 bufmgr.c
--- src/backend/storage/buffer/bufmgr.c 19 Nov 2008 10:34:52 -  1.242
+++ src/backend/storage/buffer/bufmgr.c 15 Dec 2008 05:12:45 -
@@ -203,8 +203,7 @@ ReadBuffer_common(SMgrRelation smgr, boo
if (isExtend)
blockNum = smgrnblocks(smgr, forkNum);
 
-   TRACE_POSTGRESQL_BUFFER_READ_START(blockNum, smgr-smgr_rnode.spcNode,
-   smgr-smgr_rnode.dbNode, smgr-smgr_rnode.relNode, isLocalBuf);
+   TRACE_POSTGRESQL_BUFFER_READ_START(forkNum, blockNum, 
smgr-smgr_rnode.spcNode, smgr-smgr_rnode.dbNode, smgr-smgr_rnode.relNode, 
isLocalBuf);
 
if (isLocalBuf)
{
@@ -253,7 +252,7 @@ ReadBuffer_common(SMgrRelation smgr, boo
if (VacuumCostActive)
VacuumCostBalance += VacuumCostPageHit;
 
-   TRACE_POSTGRESQL_BUFFER_READ_DONE(blockNum,
+   TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum,
smgr-smgr_rnode.spcNode,
smgr-smgr_rnode.dbNode,
smgr-smgr_rnode.relNode, isLocalBuf, found);
@@ -380,9 +379,9 @@ ReadBuffer_common(SMgrRelation smgr, boo
if (VacuumCostActive)
VacuumCostBalance += VacuumCostPageMiss;
 
-   TRACE_POSTGRESQL_BUFFER_READ_DONE(blockNum, smgr-smgr_rnode.spcNode,
-   smgr-smgr_rnode.dbNode, smgr-smgr_rnode.relNode,
-   isLocalBuf, found);
+   

Re: [HACKERS] DTrace probes patch

[Bruce Momjian]

Should I apply this or hold it for 8.5?

---

Robert Lor wrote:
 
 The attached patch contains a couple of fixes in the existing probes and 
 includes a few new ones.
 
 - Fixed compilation errors on OS X for probes that use typedefs
 - Fixed a number of probes to pass ForkNumber per the relation forks patch
 - The new probes are those that were taken out from the previous 
 submitted patch and required simple fixes. Will submit the other probes 
 that may require more discussion in a separate patch.
 
 
 -Robert
 
 
 
 

 Index: src/backend/access/transam/xlog.c
 ===
 RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/xlog.c,v
 retrieving revision 1.322
 diff -u -3 -p -r1.322 xlog.c
 --- src/backend/access/transam/xlog.c 9 Nov 2008 17:51:15 -   1.322
 +++ src/backend/access/transam/xlog.c 15 Dec 2008 05:12:41 -
 @@ -48,6 +48,7 @@
  #include utils/builtins.h
  #include utils/guc.h
  #include utils/ps_status.h
 +#include pg_trace.h
  
  
  /* File path names (all relative to $PGDATA) */
 @@ -486,6 +487,8 @@ XLogInsert(RmgrId rmid, uint8 info, XLog
   if (info  XLR_INFO_MASK)
   elog(PANIC, invalid xlog info mask %02X, info);
  
 + TRACE_POSTGRESQL_XLOG_INSERT(rmid, info);
 +
   /*
* In bootstrap mode, we don't actually log anything but XLOG resources;
* return a phony record pointer.
 @@ -914,6 +917,8 @@ begin:;
   XLogwrtRqst FlushRqst;
   XLogRecPtr  OldSegEnd;
  
 + TRACE_POSTGRESQL_XLOG_SWITCH();
 +
   LWLockAcquire(WALWriteLock, LW_EXCLUSIVE);
  
   /*
 @@ -1313,12 +1318,14 @@ AdvanceXLInsertBuffer(bool new_segment)
* Have to write buffers while holding insert 
 lock. This is
* not good, so only write as much as we 
 absolutely must.
*/
 + TRACE_POSTGRESQL_WAL_BUFFER_WRITE_START();
   WriteRqst.Write = OldPageRqstPtr;
   WriteRqst.Flush.xlogid = 0;
   WriteRqst.Flush.xrecoff = 0;
   XLogWrite(WriteRqst, false, false);
   LWLockRelease(WALWriteLock);
   Insert-LogwrtResult = LogwrtResult;
 + TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DONE();
   }
   }
   }
 @@ -5904,6 +5911,8 @@ CreateCheckPoint(int flags)
   if (log_checkpoints)
   LogCheckpointStart(flags);
  
 + TRACE_POSTGRESQL_CHECKPOINT_START(flags);
 +
   /*
* Before flushing data, we must wait for any transactions that are
* currently in their commit critical sections.  If an xact inserted its
 @@ -6069,6 +6078,11 @@ CreateCheckPoint(int flags)
   if (log_checkpoints)
   LogCheckpointEnd();
  
 +TRACE_POSTGRESQL_CHECKPOINT_DONE(CheckpointStats.ckpt_bufs_written,
 +NBuffers, CheckpointStats.ckpt_segs_added,
 +CheckpointStats.ckpt_segs_removed,
 +CheckpointStats.ckpt_segs_recycled);
 +
   LWLockRelease(CheckpointLock);
  }
  
 Index: src/backend/storage/buffer/bufmgr.c
 ===
 RCS file: /projects/cvsroot/pgsql/src/backend/storage/buffer/bufmgr.c,v
 retrieving revision 1.242
 diff -u -3 -p -r1.242 bufmgr.c
 --- src/backend/storage/buffer/bufmgr.c   19 Nov 2008 10:34:52 -  
 1.242
 +++ src/backend/storage/buffer/bufmgr.c   15 Dec 2008 05:12:45 -
 @@ -203,8 +203,7 @@ ReadBuffer_common(SMgrRelation smgr, boo
   if (isExtend)
   blockNum = smgrnblocks(smgr, forkNum);
  
 - TRACE_POSTGRESQL_BUFFER_READ_START(blockNum, smgr-smgr_rnode.spcNode,
 - smgr-smgr_rnode.dbNode, smgr-smgr_rnode.relNode, isLocalBuf);
 + TRACE_POSTGRESQL_BUFFER_READ_START(forkNum, blockNum, 
 smgr-smgr_rnode.spcNode, smgr-smgr_rnode.dbNode, smgr-smgr_rnode.relNode, 
 isLocalBuf);
  
   if (isLocalBuf)
   {
 @@ -253,7 +252,7 @@ ReadBuffer_common(SMgrRelation smgr, boo
   if (VacuumCostActive)
   VacuumCostBalance += VacuumCostPageHit;
  
 - TRACE_POSTGRESQL_BUFFER_READ_DONE(blockNum,
 + TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum,
   smgr-smgr_rnode.spcNode,
   smgr-smgr_rnode.dbNode,
   smgr-smgr_rnode.relNode, isLocalBuf, found);
 @@ -380,9 +379,9 @@ ReadBuffer_common(SMgrRelation smgr, boo
   if (VacuumCostActive)
   VacuumCostBalance += VacuumCostPageMiss;
  
 - 

Re: [HACKERS] DTrace probes.

[Robert Treat]

On Monday 19 May 2008 11:32:28 Theo Schlossnagle wrote:
 Howdy,

 I just saw Robert Lor's patch w.r.t. dtrace probes.  It looks very
 similar in what we've done.  We run a nice set of probes in production
 here that allow us to track the details of checkpointing and statement
 execution.  I've given a few presentations around these probes and
 have had very positive feedback.   They've been available for a while
 now, but I never got around to sending them to the list:

 https://labs.omniti.com/trac/project-dtrace/browser/trunk/postgresql/8.3.1.
patch?format=txt

 Documentation is in wiki format, but I'd be happy to convert it to
 something else:

 https://labs.omniti.com/trac/project-dtrace/wiki/Applications#PostgreSQL


Attached is the patch combining Theo's patch from above into the original 
patch from Robert Lor.  It is supposed to build on OSX and Solaris.  I'll be 
updating the July commitfest entry to point to this patch, case anyone wants 
to review it. 

-- 
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL
Index: src/backend/access/transam/clog.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/clog.c,v
retrieving revision 1.46
diff -u -3 -p -r1.46 clog.c
--- src/backend/access/transam/clog.c	1 Jan 2008 19:45:46 -	1.46
+++ src/backend/access/transam/clog.c	26 Jun 2008 23:18:30 -
@@ -36,6 +36,7 @@
 #include access/slru.h
 #include access/transam.h
 #include postmaster/bgwriter.h
+#include pg_trace.h
 
 /*
  * Defines for CLOG page sizes.  A page is the same BLCKSZ as is used
@@ -323,7 +324,9 @@ void
 CheckPointCLOG(void)
 {
 	/* Flush dirty CLOG pages to disk */
+	TRACE_POSTGRESQL_CLOG_CHECKPOINT_START();
 	SimpleLruFlush(ClogCtl, true);
+	TRACE_POSTGRESQL_CLOG_CHECKPOINT_DONE();
 }
 
 
Index: src/backend/access/transam/multixact.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/multixact.c,v
retrieving revision 1.27
diff -u -3 -p -r1.27 multixact.c
--- src/backend/access/transam/multixact.c	1 Jan 2008 19:45:46 -	1.27
+++ src/backend/access/transam/multixact.c	26 Jun 2008 23:18:31 -
@@ -57,6 +57,7 @@
 #include storage/lmgr.h
 #include utils/memutils.h
 #include storage/procarray.h
+#include pg_trace.h
 
 
 /*
@@ -1526,6 +1527,8 @@ MultiXactGetCheckptMulti(bool is_shutdow
 void
 CheckPointMultiXact(void)
 {
+	TRACE_POSTGRESQL_MULTIXACT_CHECKPOINT_START();
+
 	/* Flush dirty MultiXact pages to disk */
 	SimpleLruFlush(MultiXactOffsetCtl, true);
 	SimpleLruFlush(MultiXactMemberCtl, true);
@@ -1540,6 +1543,8 @@ CheckPointMultiXact(void)
 	 */
 	if (!InRecovery)
 		TruncateMultiXact();
+
+	TRACE_POSTGRESQL_MULTIXACT_CHECKPOINT_DONE();
 }
 
 /*
Index: src/backend/access/transam/slru.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/slru.c,v
retrieving revision 1.44
diff -u -3 -p -r1.44 slru.c
--- src/backend/access/transam/slru.c	1 Jan 2008 19:45:48 -	1.44
+++ src/backend/access/transam/slru.c	26 Jun 2008 23:18:32 -
@@ -57,6 +57,7 @@
 #include storage/fd.h
 #include storage/shmem.h
 #include miscadmin.h
+#include pg_trace.h
 
 
 /*
@@ -372,6 +373,8 @@ SimpleLruReadPage(SlruCtl ctl, int pagen
 {
 	SlruShared	shared = ctl-shared;
 
+	TRACE_POSTGRESQL_SLRU_READPAGE_START((uintptr_t)ctl, pageno, write_ok, xid);
+
 	/* Outer loop handles restart if we must wait for someone else's I/O */
 	for (;;)
 	{
@@ -399,6 +402,7 @@ SimpleLruReadPage(SlruCtl ctl, int pagen
 			}
 			/* Otherwise, it's ready to use */
 			SlruRecentlyUsed(shared, slotno);
+			TRACE_POSTGRESQL_SLRU_READPAGE_DONE(slotno);
 			return slotno;
 		}
 
@@ -446,6 +450,7 @@ SimpleLruReadPage(SlruCtl ctl, int pagen
 			SlruReportIOError(ctl, pageno, xid);
 
 		SlruRecentlyUsed(shared, slotno);
+		TRACE_POSTGRESQL_SLRU_READPAGE_DONE(slotno);
 		return slotno;
 	}
 }
@@ -470,6 +475,8 @@ SimpleLruReadPage_ReadOnly(SlruCtl ctl, 
 	SlruShared	shared = ctl-shared;
 	int			slotno;
 
+	TRACE_POSTGRESQL_SLRU_READPAGE_READONLY((uintptr_t)ctl, pageno, xid);
+
 	/* Try to find the page while holding only shared lock */
 	LWLockAcquire(shared-ControlLock, LW_SHARED);
 
@@ -511,6 +518,8 @@ SimpleLruWritePage(SlruCtl ctl, int slot
 	int			pageno = shared-page_number[slotno];
 	bool		ok;
 
+	TRACE_POSTGRESQL_SLRU_WRITEPAGE_START((uintptr_t)ctl, pageno, slotno);
+
 	/* If a write is in progress, wait for it to finish */
 	while (shared-page_status[slotno] == SLRU_PAGE_WRITE_IN_PROGRESS 
 		   shared-page_number[slotno] == pageno)
@@ -525,7 +534,10 @@ SimpleLruWritePage(SlruCtl ctl, int slot
 	if (!shared-page_dirty[slotno] ||
 		shared-page_status[slotno] != SLRU_PAGE_VALID ||
 		shared-page_number[slotno] != pageno)
+	{
+		TRACE_POSTGRESQL_SLRU_WRITEPAGE_DONE();
 		return;
+	}
 
 	/*
 	 * Mark the slot write-busy, and clear the dirtybit.  After this point, a
@@ -569,6 +581,8 

[HACKERS] DTrace probes.

[Theo Schlossnagle]

Howdy,

I just saw Robert Lor's patch w.r.t. dtrace probes.  It looks very  
similar in what we've done.  We run a nice set of probes in production  
here that allow us to track the details of checkpointing and statement  
execution.  I've given a few presentations around these probes and  
have had very positive feedback.   They've been available for a while  
now, but I never got around to sending them to the list:


https://labs.omniti.com/trac/project-dtrace/browser/trunk/postgresql/8.3.1.patch?format=txt

Documentation is in wiki format, but I'd be happy to convert it to  
something else:


https://labs.omniti.com/trac/project-dtrace/wiki/Applications#PostgreSQL

Best regards,

Theo


--
Theo Schlossnagle
Esoteric Curio -- http://lethargy.org/
OmniTI Computer Consulting, Inc. -- http://omniti.com/


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] DTrace Probes?

[Nicolai Tufar]

On 6/17/05, Josh Berkus josh@agliodbs.com wrote:
 Hey, Folks,
 
 I need to find someone who's really interesed in working with DTrace.  Sun
 has offered to help put DTrace probes into PostgreSQL for advanced
 profiling, but need to know where to probe.   Anyone?
 
 I'm afraid that I won't get around to this quickly enough.

I played a little with DTrace probes when Solaris 10 just came
out. DTrace is useful when you have no source code of application
or when you are collecting statistics on a live system. Otherwise
it is not much different from gprof apart, maybe, that it can collect
statistics about kernel syscalls. 

Anyways, DTrace is a very powerful yet lightweight tool. Creating a 
strace program to attach to a running PostgreSQL instance and collect 
statistics will be a nice thing to do. We may even find some bottlenecks
in our code. I can volunteer to do it but I do not have a through understanding
of PostgreSQL internals. 

 --Josh
Regards, 
Nicolai

---(end of broadcast)---
TIP 8: explain analyze is your friend

[HACKERS] DTrace Probes?

[Josh Berkus]

Hey, Folks,

I need to find someone who's really interesed in working with DTrace.  Sun 
has offered to help put DTrace probes into PostgreSQL for advanced 
profiling, but need to know where to probe.   Anyone?

I'm afraid that I won't get around to this quickly enough.

-- 
--Josh

Josh Berkus
Aglio Database Solutions
San Francisco

---(end of broadcast)---
TIP 7: don't forget to increase your free space map settings