On Fri, Dec 15, 2017 at 3:32 AM, Kyotaro HORIGUCHI <horiguchi.kyot...@lab.ntt.co.jp> 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