autoprewarm is fogetting to register a tranche.

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: pgsql-hackers(at)postgresql(dot)org
Subject: autoprewarm is fogetting to register a tranche.
Date: 2017-12-15 08:32:19
Message-ID: 20171215.173219.38055760.horiguchi.kyotaro@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
0001-Add-a-new-tranche-register-API.patch text/x-patch 2.9 KB
0002-Register-the-tranche-for-LWLock.patch text/x-patch 672 bytes
0003-Check-tranche-name-on-LWLock.patch text/x-patch 873 bytes

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Konstantin Knizhnik 2017-12-15 09:14:05 Re: [HACKERS] Surjective functional indexes
Previous Message Ashutosh Bapat 2017-12-15 05:57:19 Re: [HACKERS] Partition-wise aggregation/grouping