Re: [PATCH] Identify LWLocks in tracepoints

Hi, On 2021-05-10 09:46:02 -0700, Andres Freund wrote: > No worries - I knew that I'd have to do this at some point, even though > I hadn't planned to do that today... I should have all of them green > before end of today. > > I found that I actually can build LLVM 3.9 directly, as clang-6 can >

Re: [PATCH] Identify LWLocks in tracepoints

Hi, On 2021-05-10 12:14:46 -0400, Tom Lane wrote: > Andres Freund writes: > > Looks like it did, but turned out to have some unintended side-effects > > :(. > > The snapshot builds are now new: > > https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=flaviventris&dt=2021-05-10%2015%3A43%

Re: [PATCH] Identify LWLocks in tracepoints

Andres Freund writes: > Looks like it did, but turned out to have some unintended side-effects > :(. > The snapshot builds are now new: > https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=flaviventris&dt=2021-05-10%2015%3A43%3A56&stg=configure > configure:3966: ccache /usr/lib/gcc-snap

Re: [PATCH] Identify LWLocks in tracepoints

On 2021-05-09 19:51:13 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2021-05-08 13:13:47 -0400, Tom Lane wrote: > >> (I wonder why flaviventris and serinus are still using an "experimental" > >> compiler version that is now behind mainstream.) > > > The upgrade script didn't install the n

Re: [PATCH] Identify LWLocks in tracepoints

On 05.05.21 06:20, Craig Ringer wrote: On Wed, 5 May 2021 at 09:15, Craig Ringer wrote: warning: suggest braces around empty body in an ‘if’ statement [-Wempty-body] 1322 |TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode); |

Re: [PATCH] Identify LWLocks in tracepoints

Andres Freund writes: > On 2021-05-08 13:13:47 -0400, Tom Lane wrote: >> (I wonder why flaviventris and serinus are still using an "experimental" >> compiler version that is now behind mainstream.) > The upgrade script didn't install the newer version it because it had to > remove some conflictin

Re: [PATCH] Identify LWLocks in tracepoints

Hi, On 2021-05-08 13:13:47 -0400, Tom Lane wrote: > Peter Eisentraut writes: > > On 05.05.21 00:15, Andres Freund wrote: > >> I'm now getting > >> /home/andres/src/postgresql/src/backend/storage/lmgr/lwlock.c: In function > >> ‘LWLockAcquire’: > >> /home/andres/src/postgresql/src/backend/storage

Re: [PATCH] Identify LWLocks in tracepoints

Peter Eisentraut writes: > On 05.05.21 00:15, Andres Freund wrote: >> I'm now getting >> /home/andres/src/postgresql/src/backend/storage/lmgr/lwlock.c: In function >> ‘LWLockAcquire’: >> /home/andres/src/postgresql/src/backend/storage/lmgr/lwlock.c:1322:58: >> warning: suggest braces around empt

Re: [PATCH] Identify LWLocks in tracepoints

On 05.05.21 00:15, Andres Freund wrote: I'm now getting /home/andres/src/postgresql/src/backend/storage/lmgr/lwlock.c: In function ‘LWLockAcquire’: /home/andres/src/postgresql/src/backend/storage/lmgr/lwlock.c:1322:58: warning: suggest braces around empty body in an ‘if’ statement [-Wempty-bod

Re: [PATCH] Identify LWLocks in tracepoints

On Wed, 5 May 2021 at 09:15, Craig Ringer wrote: >> warning: suggest braces around empty body in an ‘if’ statement [-Wempty-body] >> 1322 |TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode); >> | ^ > > Odd that I didn't get t

Re: [PATCH] Identify LWLocks in tracepoints

On Wed, 5 May 2021, 06:15 Andres Freund, wrote: > Hi, > > warning: suggest braces around empty body in an ‘if’ statement > [-Wempty-body] > 1322 |TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode); > | ^ > Odd that I didn't

Re: [PATCH] Identify LWLocks in tracepoints

Hi, On 2021-05-03 21:06:30 +0200, Peter Eisentraut wrote: > On 30.04.21 05:22, Craig Ringer wrote: > > On Thu, 29 Apr 2021 at 15:31, Peter Eisentraut > > wrote: > > > > So if you could produce a separate patch that adds the > > > > _ENABLED guards targeting PG14 (and PG13), that would be helpful.

Re: [PATCH] Identify LWLocks in tracepoints

On 30.04.21 05:22, Craig Ringer wrote: On Thu, 29 Apr 2021 at 15:31, Peter Eisentraut wrote: 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. LGTM. Applies and builds fine on master a

Re: [PATCH] Identify LWLocks in tracepoints

> On Fri, Apr 30, 2021 at 11:23:56AM +0800, Craig Ringer wrote: > On Wed, 14 Apr 2021, 22:29 Robert Haas, wrote: > > > > I'm actually inclined to revise the patch I sent in order to *remove* > > > the LWLock name from the tracepoint argument. > > > Reducing the overheads is good, but I have no o

Re: [PATCH] Identify LWLocks in tracepoints

On Wed, 14 Apr 2021, 22:29 Robert Haas, wrote: > On Tue, Apr 13, 2021 at 10:42 PM Craig Ringer > wrote: > > I'd really love it if a committer could add an explanatory comment or > > two in the area though. I'm happy to draft a comment patch if anyone > > agrees my suggestion is sensible. The key

Re: [PATCH] Identify LWLocks in tracepoints

On Thu, 29 Apr 2021 at 15:31, Peter Eisentraut wrote: > > 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. LGTM. Applies and builds fine on master and (with default fuzz) on REL_13_

Re: [PATCH] Identify LWLocks in tracepoints

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.   

Re: [PATCH] Identify LWLocks in tracepoints

On Tue, Apr 13, 2021 at 10:42 PM Craig Ringer wrote: > I'd really love it if a committer could add an explanatory comment or > two in the area though. I'm happy to draft a comment patch if anyone > agrees my suggestion is sensible. The key things I needed to know when > studying the code were: > >

Re: [PATCH] Identify LWLocks in tracepoints

On Tue, Apr 13, 2021 at 4:46 PM Andres Freund wrote: > I still don't like the two bytes, fwiw ;). Especially because it's 4 > bytes due to padding right now. I'm not surprised by that disclosure. But I think it's entirely worth it. Making wait states visible in pg_stat_activity isn't the most use

Re: [PATCH] Identify LWLocks in tracepoints

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

Re: [PATCH] Identify LWLocks in tracepoints

On Wed, 14 Apr 2021 at 10:41, Craig Ringer wrote: > On Wed, 14 Apr 2021 at 02:25, Robert Haas wrote: > > You could try to identify locks by pointer addresses, but that's got > > security hazards and the addreses aren't portable across all the > > backends involved in the parallel query because of

Re: [PATCH] Identify LWLocks in tracepoints

On Wed, 14 Apr 2021 at 02:25, Robert Haas wrote: > So before the commit in question -- > 3761fe3c20bb040b15f0e8da58d824631da00caa -- T_ID() used to compute an > offset for a lock within the tranche that was supposed to uniquely > identify the lock. However, the whole idea of an array per tranche

Re: [PATCH] Identify LWLocks in tracepoints

On Wed, 14 Apr 2021 at 04:46, Andres Freund wrote: > > On 2021-04-13 14:25:23 -0400, Robert Haas wrote: > > On Mon, Apr 12, 2021 at 11:06 PM Andres Freund wrote: > > You could identify every lock by a tranche ID + an array offset + a > > "tranche instance ID". But where would you store the tranch

Re: [PATCH] Identify LWLocks in tracepoints

Hi, On 2021-04-13 14:25:23 -0400, Robert Haas wrote: > On Mon, Apr 12, 2021 at 11:06 PM Andres Freund wrote: > You could identify every lock by a tranche ID + an array offset + a > "tranche instance ID". But where would you store the tranche instance > ID to make it readily accessible, other than

Re: [PATCH] Identify LWLocks in tracepoints

On Mon, Apr 12, 2021 at 11:06 PM Andres Freund wrote: > No, they have to be the same in each. Note how the tranche ID is part of > struct LWLock. Which is why LWLockNewTrancheId() has to acquire a lock > etc. More precisely, if a tranche ID is defined in multiple backends, it needs to be defined

Re: [PATCH] Identify LWLocks in tracepoints

On Tue, 13 Apr 2021 at 21:40, Craig Ringer wrote: > Findings: > > * A probe without arguments or with simple arguments is just a 'nop' > instruction > * Probes that require function calls, pointer chasing, other > expression evaluation etc may impose a fixed cost to collect up > arguments even i

Re: [PATCH] Identify LWLocks in tracepoints

On Tue, 13 Apr 2021 at 21:05, Craig Ringer wrote: > On Tue, 13 Apr 2021 at 11:06, Andres Freund wrote: > > IIRC those aren't really comparable - the kernel actually does modify > > the executable code to replace the tracepoints with nops. > > Same with userspace static trace markers (USDTs). > >

Re: [PATCH] Identify LWLocks in tracepoints

On Tue, 13 Apr 2021 at 11:06, Andres Freund wrote: > > Each backend can have different tranche IDs (right?) > > No, they have to be the same in each. Note how the tranche ID is part of > struct LWLock. Which is why LWLockNewTrancheId() has to acquire a lock > etc. Ah. I misunderstood that at som

Re: [PATCH] Identify LWLocks in tracepoints

Hi, On 2021-04-13 10:34:18 +0800, Craig Ringer wrote: > > But it's near trivial to add that. > > Really? Yes. > Each backend can have different tranche IDs (right?) No, they have to be the same in each. Note how the tranche ID is part of struct LWLock. Which is why LWLockNewTrancheId() has to

Re: [PATCH] Identify LWLocks in tracepoints

On Tue, 13 Apr 2021 at 02:23, Andres Freund wrote: [I've changed the order of the quoted sections a little to prioritize the key stuff] > > On 2021-04-12 14:31:32 +0800, Craig Ringer wrote: > > > It's annoying that we have to pay the cost of computing the tranche name > > though. It never used t

Re: [PATCH] Identify LWLocks in tracepoints

Hi, On 2021-04-12 14:31:32 +0800, Craig Ringer wrote: > * There is no easy way to look up the tranche name by ID from outside the > backend But it's near trivial to add that. > It's annoying that we have to pay the cost of computing the tranche name > though. It never used to matter, but now th

Re: [PATCH] Identify LWLocks in tracepoints

On Mon, 22 Mar 2021 at 16:38, Peter Eisentraut < peter.eisentr...@enterprisedb.com> wrote: > > First, a problem: 0002 doesn't build on macOS, because uint64 has been > used in the probe definitions. That needs to be handled like the other > nonnative types in that file. > Will fix. All the pro

Re: [PATCH] Identify LWLocks in tracepoints

On Mon, 22 Mar 2021 at 17:00, Peter Eisentraut < peter.eisentr...@enterprisedb.com> wrote: > On 20.03.21 01:29, Craig Ringer wrote: > > There is already support for that. See the documentation at the end > of > > this page: > > > https://www.postgresql.org/docs/devel/dynamic-trace.html#DE

Re: [PATCH] Identify LWLocks in tracepoints

On 20.03.21 01:29, Craig Ringer wrote: There is already support for that.  See the documentation at the end of this page: https://www.postgresql.org/docs/devel/dynamic-trace.html#DEFINING-TRACE-POINTS

Re: [PATCH] Identify LWLocks in tracepoints

On 10.03.21 06:38, Craig Ringer wrote: On Wed, 3 Mar 2021 at 20:50, David Steele > wrote: On 1/22/21 6:02 AM, Peter Eisentraut wrote: This patch set no longer applies: http://cfbot.cputube.org/patch_32_2927.log

Re: [PATCH] Identify LWLocks in tracepoints

On 19.03.21 21:06, Peter Eisentraut wrote: On 18.03.21 07:34, Craig Ringer wrote:     In patch 0001, why was the TRACE_POSTGRESQL_LWLOCK_RELEASE() call     moved?   Is there some correctness issue?  If so, we should explain that (at     least in the commit message, or as a separate patch)

Re: [PATCH] Identify LWLocks in tracepoints

On Sat, 20 Mar 2021, 04:21 Peter Eisentraut, < peter.eisentr...@enterprisedb.com> wrote: > > On 18.03.21 07:34, Craig Ringer wrote: > > The main reason I didn't want to add more tracepoints than were strictly > > necessary is that Pg doesn't enable the systemtap semaphores feature, so > > right no

Re: [PATCH] Identify LWLocks in tracepoints

On 18.03.21 07:34, Craig Ringer wrote: The main reason I didn't want to add more tracepoints than were strictly necessary is that Pg doesn't enable the systemtap semaphores feature, so right now we do a T_NAME(lock) evaluation each time we pass a tracepoint if --enable-dtrace is compiled in,

Re: [PATCH] Identify LWLocks in tracepoints

On 18.03.21 07:34, Craig Ringer wrote: In patch 0001, why was the TRACE_POSTGRESQL_LWLOCK_RELEASE() call moved?   Is there some correctness issue?  If so, we should explain that (at least in the commit message, or as a separate patch). If you want I can split it out, or drop th

Re: [PATCH] Identify LWLocks in tracepoints

On Thu, 11 Mar 2021 at 15:57, Peter Eisentraut < peter.eisentr...@enterprisedb.com> wrote: > On 10.03.21 06:38, Craig Ringer wrote: > > On Wed, 3 Mar 2021 at 20:50, David Steele > > wrote: > > > > On 1/22/21 6:02 AM, Peter Eisentraut wrote: > > > > This patch s

Re: [PATCH] Identify LWLocks in tracepoints

On 10.03.21 06:38, Craig Ringer wrote: On Wed, 3 Mar 2021 at 20:50, David Steele > wrote: On 1/22/21 6:02 AM, Peter Eisentraut wrote: This patch set no longer applies: http://cfbot.cputube.org/patch_32_2927.log

Re: [PATCH] Identify LWLocks in tracepoints

On Wed, 3 Mar 2021 at 20:50, David Steele wrote: > On 1/22/21 6:02 AM, Peter Eisentraut wrote: > > This patch set no longer applies: > http://cfbot.cputube.org/patch_32_2927.log. > > Can we get a rebase? Also marked Waiting on Author. > Rebased as requested. I'm still interested in whether Andr

Re: [PATCH] Identify LWLocks in tracepoints

On 1/22/21 6:02 AM, Peter Eisentraut wrote: On 2021-01-14 09:39, Craig Ringer wrote: On Thu, 14 Jan 2021 at 15:56, Peter Eisentraut > wrote:     On 2020-12-19 06:00, Craig Ringer wrote: > Patch 1 fixes a bogus tracepoint where an lwlock__acquire e

Re: [PATCH] Identify LWLocks in tracepoints

On 2021-01-14 09:39, Craig Ringer wrote: On Thu, 14 Jan 2021 at 15:56, Peter Eisentraut > wrote: On 2020-12-19 06:00, Craig Ringer wrote: > Patch 1 fixes a bogus tracepoint where an lwlock__acquire event would be > fired from LWLockWai

Re: [PATCH] Identify LWLocks in tracepoints

On Thu, 14 Jan 2021 at 15:56, Peter Eisentraut < peter.eisentr...@enterprisedb.com> wrote: > On 2020-12-19 06:00, Craig Ringer wrote: > > Patch 1 fixes a bogus tracepoint where an lwlock__acquire event would be > > fired from LWLockWaitForVar, despite that function never actually > > acquiring the

Re: [PATCH] Identify LWLocks in tracepoints

On Wed, 13 Jan 2021 at 19:19, Dmitry Dolgov <9erthali...@gmail.com> wrote: > > On Sat, Dec 19, 2020 at 01:00:01PM +0800, Craig Ringer wrote: > > > > The attached patch set follows on from the discussion in [1] "Add LWLock > > blocker(s) information" by adding the actual LWLock* and the numeric > >

Re: [PATCH] Identify LWLocks in tracepoints

On 2020-12-19 06:00, Craig Ringer wrote: Patch 1 fixes a bogus tracepoint where an lwlock__acquire event would be fired from LWLockWaitForVar, despite that function never actually acquiring the lock. This was added in 68a2e52bbaf when LWLockWaitForVar() was first introduced. It looks like a

Re: [PATCH] Identify LWLocks in tracepoints

> On Sat, Dec 19, 2020 at 01:00:01PM +0800, Craig Ringer wrote: > > The attached patch set follows on from the discussion in [1] "Add LWLock > blocker(s) information" by adding the actual LWLock* and the numeric > tranche ID to each LWLock related TRACE_POSTGRESQL_foo tracepoint. > > This does not

Re: [PATCH] Identify LWLocks in tracepoints

On Sat, 19 Dec 2020 at 13:00, Craig Ringer wrote: > Hi all > > The attached patch set follows on from the discussion in [1] "Add LWLock > blocker(s) information" by adding the actual LWLock* and the numeric > tranche ID to each LWLock related TRACE_POSTGRESQL_foo tracepoint. > > I've attached a s

Re: [PATCH] Identify LWLocks in tracepoints

On Mon, 28 Dec 2020 at 20:09, Masahiko Sawada wrote: > Hi Craig, > > On Sat, Dec 19, 2020 at 2:00 PM Craig Ringer > wrote: > > > > Hi all > > > > The attached patch set follows on from the discussion in [1] "Add LWLock > blocker(s) information" by adding the actual LWLock* and the numeric > tran

Re: [PATCH] Identify LWLocks in tracepoints

Hi Craig, On Sat, Dec 19, 2020 at 2:00 PM Craig Ringer wrote: > > Hi all > > The attached patch set follows on from the discussion in [1] "Add LWLock > blocker(s) information" by adding the actual LWLock* and the numeric tranche > ID to each LWLock related TRACE_POSTGRESQL_foo tracepoint. > > T

[PATCH] Identify LWLocks in tracepoints

Hi all The attached patch set follows on from the discussion in [1] "Add LWLock blocker(s) information" by adding the actual LWLock* and the numeric tranche ID to each LWLock related TRACE_POSTGRESQL_foo tracepoint. This does not provide complete information on blockers, because it's not necessar