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

At Mon, 06 Mar 2017 15:44:52 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20170306.154452.254472341.horiguchi.kyotaro@lab.ntt.co.jp>
> 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.

It's wrong. The module knows the allocated tranche id so it can
register the tranche properly.

> > > 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.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center