Re: autoprewarm is fogetting to register a tranche.

[Robert Haas]

On Fri, Dec 22, 2017 at 12:13 AM, Kyotaro HORIGUCHI
 wrote:
> The attached one-liner patch is just adding tranche registration
> to autprewarm.c.

Committed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: autoprewarm is fogetting to register a tranche.

[Kyotaro HORIGUCHI]

Hello,

At Mon, 18 Dec 2017 12:46:02 -0500, Robert Haas  wrote 
in 
> On Fri, Dec 15, 2017 at 3:32 AM, Kyotaro HORIGUCHI
>  wrote:
> > Hello, I noticed while an investigation that pg_prewarm is
> > forgetting to register a tranche.
> 
> Oops.
> 
> > 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)
> 
> I don't think we really need this.  If it suits a particular caller to
> to pass somelock->tranche to LWLockRegisterTranche, fine, but they can
> do that with or without this function.  It's basically a one-line
> function, so I don't see the point.

Hmm..ok, Still no point adding tranche id in the shared struct, I
changed that with using ->tranche explicitly.

> > 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)
> 
> I don't really see a need to change this.  It's true that what's
> currently #3 could be done before #2, but I hesitate to call that a
> typical practice.  Also, I'm worried that it could create the
> impression that it's OK to use an LWLock before registering the
> tranche, and it's really not.

Yeah, I basically feel the same. But I'afraid that many have used
it in the reverse order. (Especially I saw some builtin lock ()
is actually used before creating LWLockTrancheArray..)

> > 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.
> 
> Actually, I think this is a good idea as long as it doesn't hurt
> performance too much.  It catches something that would otherwise be
> hard to check.

If we could enforce LWLockInitialize() is done after
RegisterLWLockTranche(), but currently ReigserLWLockTranche is
needed in every process (and this is quite bug-prone). We could
resolve that by placing tranche list on shared memory using
DSA. Anyway this is another problem.

The attached one-liner patch is just adding tranche registration
to autprewarm.c.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
*** a/contrib/pg_prewarm/autoprewarm.c
--- b/contrib/pg_prewarm/autoprewarm.c
***
*** 767,772  apw_init_shmem(void)
--- 767,774 
  	}
  	LWLockRelease(AddinShmemInitLock);
  
+ 	LWLockRegisterTranche(apw_state->lock.tranche, "autoprewarm");
+ 
  	return found;
  }
  

Re: autoprewarm is fogetting to register a tranche.

[Robert Haas]

On Fri, Dec 15, 2017 at 3:32 AM, Kyotaro HORIGUCHI
 wrote:
> Hello, I noticed while an investigation that pg_prewarm is
> forgetting to register a tranche.

Oops.

> 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)

I don't think we really need this.  If it suits a particular caller to
to pass somelock->tranche to LWLockRegisterTranche, fine, but they can
do that with or without this function.  It's basically a one-line
function, so I don't see the point.

> 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)

I don't really see a need to change this.  It's true that what's
currently #3 could be done before #2, but I hesitate to call that a
typical practice.  Also, I'm worried that it could create the
impression that it's OK to use an LWLock before registering the
tranche, and it's really not.

> 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.

Actually, I think this is a good idea as long as it doesn't hurt
performance too much.  It catches something that would otherwise be
hard to check.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

autoprewarm is fogetting to register a tranche.

[Kyotaro HORIGUCHI]

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 
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