Re: autoprewarm is fogetting to register a tranche.
On Fri, Dec 22, 2017 at 12:13 AM, Kyotaro HORIGUCHIwrote: > 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.
Hello, At Mon, 18 Dec 2017 12:46:02 -0500, Robert Haaswrote 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.
On Fri, Dec 15, 2017 at 3:32 AM, Kyotaro HORIGUCHIwrote: > 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.
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 HoriguchiDate: 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