autoprewarm is fogetting to register a tranche. - Mailing list pgsql-hackers

Hello, I noticed while an investigation that pg_prewarm is
forgetting to register a tranche.

Before the second parameter of LWLockRegisterTranche() became
char * in 3761fe3, that would lead to a crash for a
--enable-dtrace build, but currently it not likely.

On the other hand as far as reading a comment in lwlock.h, we
ought to register a tranche obtained by LWLockNewTrancheId() and
it is still convincing. So I made autoprewarm.c register
it. (patch 0002)

> * There is another, more flexible method of obtaining lwlocks. First, call
> * LWLockNewTrancheId just once to obtain a tranche ID; this allocates from a
> * shared counter. Next, LWLockInitialize should be called just once per
> * lwlock, passing the tranche ID as an argument. Finally, each individual
> * process using the tranche should call LWLockRegisterTranche() or
> * LWLockRegisterTrancheOfLock() to associate that tranche ID with a name.

In passing, I found it hard to register a tranche in
apw_init_shmem() because a process using the initialized shared
memory struct cannot find the tranche id (without intruding into
the internal of LWLock strcut). So I'd like to propose another
tranche registering interface LWLockRegisterTrancheOfLock(LWLock
*, int). The interface gets rid of the necessity of storing 
tranche id separately from LWLock. (in patch 0001)

The comment cited above looks a bit different from how the
interface is actually used. So I rearranged the comment following
a typical use I have seen in several cases. (in patch 0001)

+ * There is another, more flexible method of obtaining lwlocks. First, call
+ * LWLockNewTrancheId just once to obtain a tranche ID; this allocates from a
+ * shared counter. Next, LWLockInitialize should be called just once per
+ * lwlock, passing the tranche ID as an argument. Finally, each individual
+ * process using the tranche should call LWLockRegisterTranche() or
+ * LWLockRegisterTrancheOfLock() to associate that tranche ID with a name.

This might seem somewhat odd but things should happen in the
order in real codes since tranche registration usulally happens
after a lock initializing block.


The final patch 0003 should be arguable, or anticipated to be
rejected. It cannot be detect that a tranche is not registered
until its name is actually accessed (or many eewon't care even if
the name were printed as '(null)' in an error message that is the
result of the developer's own bug). On the other hand there's no
chance to detect that before the lock is actually used. By the
last patch I added an assertion in LWLockAcquire() but it must
hit performance in certain dgree (even if it is only in
--enable-cassert build) so I don't insisit that it must be there.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 3ac3de48c9c6992bf9137dc65362ada502100c3b Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Fri, 15 Dec 2017 14:08:18 +0900
Subject: [PATCH 1/3] Add a new tranche register API

In a typical usage of named tranches, it would be useful if we can
register a tranche not needing remember the tranche id separately in
shared memory. This new function accepts LWLock itself to specify the
tranche id.
---
 src/backend/storage/lmgr/lwlock.c | 15 +++++++++++++++
 src/include/storage/lwlock.h      | 11 ++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 46f5c42..db197a6 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -619,6 +619,21 @@ LWLockRegisterTranche(int tranche_id, const char *tranche_name)
 }
 
 /*
+ * Register the tranche ID of the specified lock in the lookup table for the
+ * current process.
+ */
+void
+LWLockRegisterTrancheOfLock(LWLock *lock, const char *tranche_name)
+{
+    /*
+     * Different from LWLockRegisterTranche, users can easily give a LWLock of
+     * builtin tranches.
+     */
+    Assert(lock->tranche >= LWTRANCHE_FIRST_USER_DEFINED);
+    LWLockRegisterTranche(lock->tranche, tranche_name);
+}
+
+/*
  * RequestNamedLWLockTranche
  *        Request that extra LWLocks be allocated during postmaster
  *        startup.
diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index 460843d..9f02329 100644
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -172,11 +172,11 @@ extern LWLockPadded *GetNamedLWLockTranche(const char *tranche_name);
 
 /*
  * There is another, more flexible method of obtaining lwlocks. First, call
- * LWLockNewTrancheId just once to obtain a tranche ID; this allocates from
- * a shared counter.  Next, each individual process using the tranche should
- * call LWLockRegisterTranche() to associate that tranche ID with a name.
- * Finally, LWLockInitialize should be called just once per lwlock, passing
- * the tranche ID as an argument.
+ * LWLockNewTrancheId just once to obtain a tranche ID; this allocates from a
+ * shared counter. Next, LWLockInitialize should be called just once per
+ * lwlock, passing the tranche ID as an argument. Finally, each individual
+ * process using the tranche should call LWLockRegisterTranche() or
+ * LWLockRegisterTrancheOfLock() to associate that tranche ID with a name.
  *
  * It may seem strange that each process using the tranche must register it
  * separately, but dynamic shared memory segments aren't guaranteed to be
@@ -185,6 +185,7 @@ extern LWLockPadded *GetNamedLWLockTranche(const char *tranche_name);
  */
 extern int    LWLockNewTrancheId(void);
 extern void LWLockRegisterTranche(int tranche_id, const char *tranche_name);
+extern void LWLockRegisterTrancheOfLock(LWLock *lock, const char *tranche_name);
 extern void LWLockInitialize(LWLock *lock, int tranche_id);
 
 /*
-- 
2.9.2

From 89939ea23049202bba8ed4ad9380bce94f9ed0c5 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Fri, 15 Dec 2017 14:11:41 +0900
Subject: [PATCH 2/3] Register the tranche for LWLock.

---
 contrib/pg_prewarm/autoprewarm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index 006c315..a14c4ea 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -767,6 +767,8 @@ apw_init_shmem(void)
     }
     LWLockRelease(AddinShmemInitLock);
 
+    LWLockRegisterTrancheOfLock(&apw_state->lock, "autoprewarm");
+
     return found;
 }
 
-- 
2.9.2

From d482d1cefab3575104aa179e7d44bfaa84d75ff5 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Fri, 15 Dec 2017 13:25:11 +0900
Subject: [PATCH 3/3] Check tranche name on LWLock

---
 src/backend/storage/lmgr/lwlock.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index db197a6..bcb67be 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -1159,6 +1159,8 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
      * to catch unsafe coding practices.
      */
     Assert(!(proc == NULL && IsUnderPostmaster));
+    /* And the tranche for this lock must have a tranche name */
+    Assert(!LWLockTrancheArray || T_NAME(lock));
 
     /* Ensure we will have room to remember the lock */
     if (num_held_lwlocks >= MAX_SIMUL_LWLOCKS)
-- 
2.9.2