Re: [HACKERS] [BUG FIX] Removing NamedLWLockTrancheArray - Mailing list pgsql-hackers

Hello,

At Sat, 4 Mar 2017 10:07:50 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
<CAA4eK1J0DcZun00PwSiftvUjpGfD2zq8CYXv9RYtiJPGbraPTw@mail.gmail.com>
> On Fri, Mar 3, 2017 at 3:49 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> >> You can read about usage of LWLocks in extensions from below location:
> >> https://www.postgresql.org/docs/devel/static/xfunc-c.html#idp86986416
> >
> > Thank you for the pointer. I understand that the document describes the only
> > correct way to use LWLock in extensions  and using LWLockRegisterTranche  is
> > a non-standard or prohibit way to do that.
> >
> > By the way, in the case of orafce, it uses LWLockRegisterTranche  directly
> > but only when !found. So if any backend other than the creator of the shmem
> > want to access tranche, the puch tranche is not found on the process and
> > crashes. I think this is it.
> >

(I can't recall what the 'the puch' was...)

> Yeah and I think that is expected if you use LWLockRegisterTranche.
> You might want to read comments in lwlock.h to know the reason behind
> the same.

Ah, yes. I understood that. And even if a module prudently
registers its own tranche, it would be easily broken by some
other modules' omission to call LWLockNewTransactionId() for all
backends. As the result extension modules should not go that way.

> > If no other modules is installed, registeriing a tranche even if found will
> > supress the crash but it is not a solution at all.
> >
> > At least for 9.6 or 10, orafce should do that following the documentation.
> >
> 
> Agreed.
> 
> > But it still can crash from the problem by the separate
> > NamedLWLockTrancheArray. (ID range check in LWLockInitialize would be
> > useless if it is not used by extensions)
> >
> 
> What exact problem are you referring here?

At many places in lwlock.c, especially on TRACE_POSTGRESQ_*()
macros, T_NAME(lock) is used and the macro just uses tranche id
as index of the main tranche array (LWLockTrancheArray). On the
other hand it is beyond the end of of the array for the "named
tranche"s and can raise SEGV.


I can guess two ways to fix this. One is change the definition of
T_NAME.

| #define T_NAME(l) \
|   ((l)->tranche < LWTRANCHE_FIRST_USER_DEFINED ? \
|    LWLockTrancheArray[(l)->tranche] : \
|    NamedLWLockTrancheArray[(l)->tranche - LWTRANCHE_FIRST_USER_DEFINED]

It makes the patch small but I don't thing the shape is
desirable.

Then, the other way is registering named tranches into the main
tranche array. The number and names of the requested named
tranches are known to postmaster so they can be allocated and
initialized at the time.

The mapping of the shared memory is inherited to backends so
pointing to the addresses in shared memory will work in the
!EXEC_BACKEND case. I confirmed that the behavior is ensured also
in EXEC_BACKEND case.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center