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

Hello,

At Mon, 18 Dec 2017 12:46:02 -0500, Robert Haas <robertmhaas@gmail.com> wrote in
<CA+Tgmob-0TYmQsjWpGV7DTEQtRnSg-UagptzKHKGNS22HQPeuw@mail.gmail.com>
> On Fri, Dec 15, 2017 at 3:32 AM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@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.

Hmm..ok, Still no point adding tranche id in the shared struct, I
changed that with using <the lock>->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;
  }