Re: [HACKERS] dtrace probes
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
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 mailing list (pgsql-hackers@postgresql.org) To make changes to
Re: [HACKERS] dtrace probes
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
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] location #1 0x500d26 argument #1 1 signed bytes @ 0
Re: [HACKERS] Dtrace probes documentation
Tom Lane 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
Joshua Tolley 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
Re: [HACKERS] Dtrace probes documentation
On Thu, May 28, 2009 at 06:28:14PM -0400, Tom Lane wrote: > Joshua Tolley 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 documentation
Joshua Tolley 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
[HACKERS] Dtrace probes documentation
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, - transaction-start + transaction__start (LocalTransactionId) Probe that fires at the start of a new transaction. arg0 is the transaction id. - transaction-commit + transaction__commit (LocalTransactionId) Probe that fires when a transaction completes successfully. arg0 is the transaction id. - transaction-abort + transaction__abort (LocalTransactionId) Probe that fires when a transaction completes unsuccessfully. arg0 is the transaction id. - query-start + query__start (const char *) Probe that fires when the processing of a query is started. arg0 is the query string. - query-done + query__done (const char *) Probe that fires when the processing of a query is complete. arg0 is the query string. - query-parse-start + query__parse__start (const char *) Probe that fires when the parsing of a query is started. arg0 is the query string. - query-parse-done + query__parse__done (const char *) Probe that fires when the parsing of a query is complete. arg0 is the query string. - query-rewrite-start + query__rewrite__start (const char *) Probe that fires when the rewriting of a query is started. arg0 is the query string. - query-rewrite-done + query__rewrite__done (const char *) Probe that fires when the rewriting of a query is complete. arg0 is the query string. - query-plan-start + query__plan__start () Probe that fires when the planning of a query is started. - query-plan-done + query__plan__done () Probe that fires when the planning of a query is complete. - query-execute-start + query__execute__start () Probe that fires when the execution of a query is started. - query-execute-done + query__execute__done () Probe that fires when the execution of a query is complete. - statement-status + statement__status (const char *) Probe that fires anytime the server process updates its pg_stat_activity.current_query status. arg0 is the new status string. - checkpoint-start + checkpoint__start (int) Probe that fires when a checkpoint is started. arg0 holds the bitwise flags used to distinguish different checkpoint types, such as shutdown, immediate or force. - checkpoint-done + checkpoint__done (int, int, int, int, int) Probe 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. - clog-checkpoint-start + clog__checkpoint__start (bool) Probe that fires when the CLOG portion of a checkpoint is started. arg0 is true for normal checkpoint, false for shutdown checkpoint. - clog-checkpoint-done + clog__checkpoint__done (bool) Probe that fires when the CLOG portion of a checkpoint is - complete. arg0 has the same meaning as for clog-checkpoint-start. + complete. arg0 has the same meaning as for clog__checkpoint__start. - subtrans-checkpoint-start + subtrans__checkpoint__start (bool) Probe that fires when the SUBTRANS portion of a checkpoint is started. @@ -1188,14 +1188,14 @@ SELECT pg_stat_get_backend_pid(s.backendid) AS procpid, checkpoint. - subtrans-checkpoint-done + subtrans__checkpoint__done (bool) Probe that fires when the SUBTRANS portion of a checkpoint is complete. arg0 has the same meaning as for - subtrans-checkpoint-start. + subtrans__checkpoint__start. - multixact-checkpoint-start + multixact__checkpoint__start (bool) Probe that fires when the MultiXact portion of a checkpoint is started. @@ -1203,14 +1203,14 @@ SELECT pg_stat_get_backend_pid(s.
Re: [HACKERS] DTrace probes broken in HEAD on Solaris?
Robert Lor 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?
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?
Greg Stark writes: > On Tue, Mar 24, 2009 at 9:31 PM, Robert Lor 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?
On Tue, Mar 24, 2009 at 10:53 PM, Martijn van Oosterhout 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?
On Tue, Mar 24, 2009 at 10:18:08PM +, Greg Stark wrote: > On Tue, Mar 24, 2009 at 10:12 PM, Martijn van Oosterhout > 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 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?
On Tue, Mar 24, 2009 at 10:12 PM, Martijn van Oosterhout 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?
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 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?
On Tue, Mar 24, 2009 at 9:31 PM, Robert Lor 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?
Tom Lane wrote: Zdenek Kotala 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?
Dne 17.03.09 19:49, Tom Lane napsal(a): Zdenek Kotala 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 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
Re: [HACKERS] DTrace probes broken in HEAD on Solaris?
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 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
[HACKERS] DTrace probes broken in HEAD on Solaris?
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
Thanks, applied. --- Robert Lor wrote: > Tom Lane wrote: > > Robert Lor 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 http://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
Tom Lane wrote: Robert Lor 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 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 wrote: "Fujii Masao" 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
"Fujii Masao" writes: > On Thu, Dec 18, 2008 at 4:49 AM, Robert Lor 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
Hi, On Thu, Dec 18, 2008 at 4:49 AM, Robert Lor 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
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
Robert Lor escribió: > Fujii Masao wrote: >> Hi, >> >> On Wed, Dec 17, 2008 at 4:53 AM, Robert Lor 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
Fujii Masao wrote: Hi, On Wed, Dec 17, 2008 at 4:53 AM, Robert Lor 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
Hi, On Wed, Dec 17, 2008 at 4:53 AM, Robert Lor 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
Re: [HACKERS] DTrace probes patch
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, > - isL
Re: [HACKERS] DTrace probes patch
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 BufferDescriptorGetB
Re: [HACKERS] DTrace probes patch
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
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 http://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
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
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
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
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
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,
[HACKERS] DTrace probes patch
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,
Re: [HACKERS] DTrace probes.
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 t
[HACKERS] DTrace probes.
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?
On 6/17/05, Josh Berkus 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?
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