Re: autoprewarm is fogetting to register a tranche.

Hello,

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