On 14.04.21 15:20, Peter Eisentraut wrote:
On 12.04.21 07:46, Craig Ringer wrote:
> To use systemtap semaphores (the _ENABLED macros) you need to run
dtrace
> -g to generate a probes.o then link that into postgres.
>
> I don't think we do that. I'll double check soon.
We do that. (It's -G.)
Huh. I could've sworn we didn't. My mistake, it's there in
src/backend/Makefile .
In that case I'll amend the patch to use semaphore guards.
This whole thread is now obviously moved to consideration for PG15, but
I did add an open item about this particular issue
(https://wiki.postgresql.org/wiki/PostgreSQL_14_Open_Items, search for
"dtrace"). So if you could produce a separate patch that adds the
_ENABLED guards targeting PG14 (and PG13), that would be helpful.
Here is a proposed patch for this.
From cae610f0203ba485770e4d274378f3ab198dcc27 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Thu, 29 Apr 2021 09:26:40 +0200
Subject: [PATCH] Prevent lwlock dtrace probes from unnecessary work
If dtrace is compiled in but disabled, the lwlock dtrace probes still
evaluate their arguments. Since PostgreSQL 13, T_NAME(lock) does
nontrivial work, so it should be avoided if not needed. To fix, make
these calls conditional on the *_ENABLED() macro corresponding to each
probe.
Reported-by: Craig Ringer <craig.rin...@enterprisedb.com>
Discussion:
https://www.postgresql.org/message-id/cagry4nwxkus_rvxfw-ugrzbyxpffm5kjwkt5o+0+stuga5b...@mail.gmail.com
---
src/backend/storage/lmgr/lwlock.c | 36 ++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/src/backend/storage/lmgr/lwlock.c
b/src/backend/storage/lmgr/lwlock.c
index 975d547f34..55b9d7970e 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -1318,7 +1318,8 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
#endif
LWLockReportWaitStart(lock);
- TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode);
+ if (TRACE_POSTGRESQL_LWLOCK_WAIT_START_ENABLED())
+ TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode);
for (;;)
{
@@ -1340,7 +1341,8 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
}
#endif
- TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
+ if (TRACE_POSTGRESQL_LWLOCK_WAIT_DONE_ENABLED())
+ TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
LWLockReportWaitEnd();
LOG_LWDEBUG("LWLockAcquire", lock, "awakened");
@@ -1349,7 +1351,8 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
result = false;
}
- TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(lock), mode);
+ if (TRACE_POSTGRESQL_LWLOCK_ACQUIRE_ENABLED())
+ TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(lock), mode);
/* Add lock to list of locks held by this backend */
held_lwlocks[num_held_lwlocks].lock = lock;
@@ -1400,14 +1403,16 @@ LWLockConditionalAcquire(LWLock *lock, LWLockMode mode)
RESUME_INTERRUPTS();
LOG_LWDEBUG("LWLockConditionalAcquire", lock, "failed");
- TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(T_NAME(lock), mode);
+ if (TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL_ENABLED())
+ TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(T_NAME(lock),
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);
+ if (TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_ENABLED())
+ TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE(T_NAME(lock), mode);
}
return !mustwait;
}
@@ -1479,7 +1484,8 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
#endif
LWLockReportWaitStart(lock);
- TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode);
+ if (TRACE_POSTGRESQL_LWLOCK_WAIT_START_ENABLED())
+
TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode);
for (;;)
{
@@ -1497,7 +1503,8 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
Assert(nwaiters < MAX_BACKENDS);
}
#endif
- TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
+ if (TRACE_POSTGRESQL_LWLOCK_WAIT_DONE_ENABLED())
+ TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock),
mode);
LWLockReportWaitEnd();
LOG_LWDEBUG("LWLockAcquireOrWait", lock, "awakened");
@@ -1527,7 +1534,8 @@ 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);
+ if (TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT_FAIL_ENABLED())
+
TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT_FAIL(T_NAME(lock), mode);
}
else
{
@@ -1535,7 +1543,8 @@ 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);
+ if (TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT_ENABLED())
+ TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT(T_NAME(lock),
mode);
}
return !mustwait;
@@ -1695,7 +1704,8 @@ LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64
oldval, uint64 *newval)
#endif
LWLockReportWaitStart(lock);
- TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), LW_EXCLUSIVE);
+ if (TRACE_POSTGRESQL_LWLOCK_WAIT_START_ENABLED())
+ TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock),
LW_EXCLUSIVE);
for (;;)
{
@@ -1714,7 +1724,8 @@ LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64
oldval, uint64 *newval)
}
#endif
- TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), LW_EXCLUSIVE);
+ if (TRACE_POSTGRESQL_LWLOCK_WAIT_DONE_ENABLED())
+ TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock),
LW_EXCLUSIVE);
LWLockReportWaitEnd();
LOG_LWDEBUG("LWLockWaitForVar", lock, "awakened");
@@ -1840,7 +1851,8 @@ LWLockRelease(LWLock *lock)
/* nobody else can have that kind of lock */
Assert(!(oldstate & LW_VAL_EXCLUSIVE));
- TRACE_POSTGRESQL_LWLOCK_RELEASE(T_NAME(lock));
+ if (TRACE_POSTGRESQL_LWLOCK_RELEASE_ENABLED())
+ TRACE_POSTGRESQL_LWLOCK_RELEASE(T_NAME(lock));
/*
* We're still waiting for backends to get scheduled, don't wake them up
base-commit: 3a948ea0a2ced719f26e725b030558f2e4ab1d8e
--
2.31.1
