Re: [BUG FIX] Removing NamedLWLockTrancheArray

At Mon, 06 Mar 2017 15:44:52 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20170306(dot)154452(dot)254472341(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> Hello,
>
> At Sat, 4 Mar 2017 10:07:50 +0530, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote in <CAA4eK1J0DcZun00PwSiftvUjpGfD2zq8CYXv9RYtiJPGbraPTw(at)mail(dot)gmail(dot)com>
> > On Fri, Mar 3, 2017 at 3:49 PM, Kyotaro HORIGUCHI
> > <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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