Thread: [HACKERS] [BUG FIX] Removing NamedLWLockTrancheArray

Hello, some of my collegues found that orafce crashes with
postgresql compliled with dtrace.

=== The cause

The immediate cause was I think that it just did
LWLockNewTrancheId and forget to do LWLockRegisterTranche. But
this is hardly detectable by a module developer.


Typical tracepoint looks like the following.

> TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);

Where T_NAME is (LWLockTrancheArray[(lock)->tranche]), as you see
T_NAME assumes that all tranches pointed from lwlocks are
available on the array but the tranches without
LWLockRegisterTranche are not. What is worse this doesn't harm so
much without dtrace (or LWLOCK_STATS or error in LWLockRelease)
. Even if dtrace is activated one or two unregistred tranches
(faultly) have their seat (filled with trash) at the end of the
array with the current code.

As my understanding there are two ways to use lwlocks in
extension.  One is using LWLockNewTrancheId and
LWLockRegisterTanche on shared memory provided by the module. The
other is using RequestNamedLWLockTranche in _PG_init and
GetNamedLWLockTranche to acquire locks provided by postgresql.

I couldn't find a documentation about lwlock and trance in
extentions, is there any?


=== How to fix this

The most straightforward way to fix this is chekcing the validity
of tranche id on initilization of a lwlock. Even though I think
that degradation won't be a matter here, NamedLWLockTrancheArray
makes the things very ineffective. After some consideration I
decided to remove NamedLWLockTrancheArray.


=== The patch

The attached patch (is for current master) is aming to fix all
the problem by doing the following two things.

- Remove NameLWLockTrancheArray and all tranches are registered in LWLockTrancheArray. This seems to work at least for
!EXEC_BAKCENDenvironment but I haven't tested with EXEC_BACKEND.
 

- Check tranche id in LWLockInitialize.

The first one required refactoring of CreateLWLocks. It is
changed to register tranches first then initialize lwlokcs.

The problem is found with PG9.6 and this should be backpatched at
least to the version. I haven't tested PG9.5 and 9.4 but it seems
to need different amendment. 9.3 doesn't has tranche.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Fri, 03 Mar 2017 17:13:42 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20170303.171342.134582021.horiguchi.kyotaro@lab.ntt.co.jp>
> Hello, some of my collegues found that orafce crashes with
> postgresql compliled with dtrace.
> 
> === The cause
> 
> The immediate cause was I think that it just did
> LWLockNewTrancheId and forget to do LWLockRegisterTranche. But
> this is hardly detectable by a module developer.

I'm sorry, I found that orafce is calling LWLockRegisterTranche
so this might be a different problem but anyway the problem with
RequestNamedLWLockTranche occurs.

> 
> Typical tracepoint looks like the following.
> 
> > TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
> 
> Where T_NAME is (LWLockTrancheArray[(lock)->tranche]), as you see
> T_NAME assumes that all tranches pointed from lwlocks are
> available on the array but the tranches without
> LWLockRegisterTranche are not. What is worse this doesn't harm so
> much without dtrace (or LWLOCK_STATS or error in LWLockRelease)
> . Even if dtrace is activated one or two unregistred tranches
> (faultly) have their seat (filled with trash) at the end of the
> array with the current code.
> 
> As my understanding there are two ways to use lwlocks in
> extension.  One is using LWLockNewTrancheId and
> LWLockRegisterTanche on shared memory provided by the module. The
> other is using RequestNamedLWLockTranche in _PG_init and
> GetNamedLWLockTranche to acquire locks provided by postgresql.
> 
> I couldn't find a documentation about lwlock and trance in
> extentions, is there any?
> 
> 
> === How to fix this
> 
> The most straightforward way to fix this is chekcing the validity
> of tranche id on initilization of a lwlock. Even though I think
> that degradation won't be a matter here, NamedLWLockTrancheArray
> makes the things very ineffective. After some consideration I
> decided to remove NamedLWLockTrancheArray.
> 
> 
> === The patch
> 
> The attached patch (is for current master) is aming to fix all
> the problem by doing the following two things.
> 
> - Remove NameLWLockTrancheArray and all tranches are registered
>   in LWLockTrancheArray. This seems to work at least for
>   !EXEC_BAKCEND environment but I haven't tested with EXEC_BACKEND.
> 
> - Check tranche id in LWLockInitialize.
> 
> The first one required refactoring of CreateLWLocks. It is
> changed to register tranches first then initialize lwlokcs.
> 
> The problem is found with PG9.6 and this should be backpatched at
> least to the version. I haven't tested PG9.5 and 9.4 but it seems
> to need different amendment. 9.3 doesn't has tranche.
> 
> regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Sorry for frequent mails..

At Fri, 03 Mar 2017 17:21:21 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20170303.172121.140674354.horiguchi.kyotaro@lab.ntt.co.jp>
> At Fri, 03 Mar 2017 17:13:42 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote
in<20170303.171342.134582021.horiguchi.kyotaro@lab.ntt.co.jp>
 
> > Hello, some of my collegues found that orafce crashes with
> > postgresql compliled with dtrace.
> > 
> > === The cause
> > 
> > The immediate cause was I think that it just did
> > LWLockNewTrancheId and forget to do LWLockRegisterTranche. But
> > this is hardly detectable by a module developer.
> 
> I'm sorry, I found that orafce is calling LWLockRegisterTranche
> so this might be a different problem but anyway the problem with
> RequestNamedLWLockTranche occurs.

As for orafce, the real cuase seems to be calling
LWLockRagisterTranche with tranche on non-static memory.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

I should be very tired.

This is the last mail today.

At Fri, 03 Mar 2017 17:26:27 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20170303.172627.225604431.horiguchi.kyotaro@lab.ntt.co.jp>
> > > Hello, some of my collegues found that orafce crashes with
> > > postgresql compliled with dtrace.
> > > 
> > > === The cause
> > > 
> > > The immediate cause was I think that it just did
> > > LWLockNewTrancheId and forget to do LWLockRegisterTranche. But
> > > this is hardly detectable by a module developer.
> > 
> > I'm sorry, I found that orafce is calling LWLockRegisterTranche
> > so this might be a different problem but anyway the problem with
> > RequestNamedLWLockTranche occurs.
> 
> As for orafce, the real cuase seems to be calling
> LWLockRagisterTranche with tranche on non-static memory.

No, it is giving static variable. Sorry for the noise.

The problem of postgresql is still exists, though.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Mar 3, 2017 at 1:43 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> Hello, some of my collegues found that orafce crashes with
> postgresql compliled with dtrace.
>
> === The cause
>
> The immediate cause was I think that it just did
> LWLockNewTrancheId and forget to do LWLockRegisterTranche. But
> this is hardly detectable by a module developer.
>
>
> Typical tracepoint looks like the following.
>
>> TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
>
> Where T_NAME is (LWLockTrancheArray[(lock)->tranche]), as you see
> T_NAME assumes that all tranches pointed from lwlocks are
> available on the array but the tranches without
> LWLockRegisterTranche are not. What is worse this doesn't harm so
> much without dtrace (or LWLOCK_STATS or error in LWLockRelease)
> . Even if dtrace is activated one or two unregistred tranches
> (faultly) have their seat (filled with trash) at the end of the
> array with the current code.
>
> As my understanding there are two ways to use lwlocks in
> extension.  One is using LWLockNewTrancheId and
> LWLockRegisterTanche on shared memory provided by the module. The
> other is using RequestNamedLWLockTranche in _PG_init and
> GetNamedLWLockTranche to acquire locks provided by postgresql.
>
> I couldn't find a documentation about lwlock and trance in
> extentions, is there any?
>

You can read about usage of LWLocks in extensions from below location:
https://www.postgresql.org/docs/devel/static/xfunc-c.html#idp86986416

Alternatively, you can check the commit
c1772ad9225641c921545b35c84ee478c326b95e to see how it is used in
pg_stat_statements.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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.
>

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.

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



-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

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

Ok, I think I understand the complete picture.

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

But this doesn't work for
LWLockNewTrancheId/LWLockRegisterTranche and it is valuable
interface. So the measure we can take is redefining T_NAME.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index ab81d94..7c4c8f4 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -115,7 +115,9 @@ static char **LWLockTrancheArray = NULL;static int    LWLockTranchesAllocated = 0;#define
T_NAME(lock)\
 
-    (LWLockTrancheArray[(lock)->tranche])
+    ((lock)->tranche < LWTRANCHE_FIRST_USER_DEFINED ? \
+    LWLockTrancheArray[(lock)->tranche] : \
+     NamedLWLockTrancheArray[(lock)->tranche - LWTRANCHE_FIRST_USER_DEFINED].trancheName)/* * This points to the main
arrayof LWLocks in shared memory.  Backends inherit 

By the way,

At Mon, 06 Mar 2017 17:07:55 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20170306.170755.68410634.horiguchi.kyotaro@lab.ntt.co.jp>
> Ok, I think I understand the complete picture.
> 
> At Mon, 06 Mar 2017 15:58:56 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote
in<20170306.155856.198084190.horiguchi.kyotaro@lab.ntt.co.jp>
 
> > > 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.
> 
> But this doesn't work for
> LWLockNewTrancheId/LWLockRegisterTranche and it is valuable
> interface. So the measure we can take is redefining T_NAME.

I realized that *I* have used the interface
RequestNamedLWLockTranche in certain extensions but I completely
forgot that and faintly recalled after being pointed by one of my
colleagues..

Sorry for the off-topic.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Mar 6, 2017 at 1:37 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> Ok, I think I understand the complete picture.
>
> At Mon, 06 Mar 2017 15:58:56 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote
in<20170306.155856.198084190.horiguchi.kyotaro@lab.ntt.co.jp>
 
>> > 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.
>
> But this doesn't work for
> LWLockNewTrancheId/LWLockRegisterTranche and it is valuable
> interface. So the measure we can take is redefining T_NAME.
>

In RegisterLWLockTranches(), we do register the named tranches as well
which should make it available in LWLockTrancheArray.  Do you see any
reason why that shouldn't work?


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

At Mon, 6 Mar 2017 19:32:25 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
<CAA4eK1LdjtsN3tpB0HJW0vfahd=Hgoq5LRy=eNh1CJZ=0WmCBg@mail.gmail.com>
> In RegisterLWLockTranches(), we do register the named tranches as well
> which should make it available in LWLockTrancheArray.  Do you see any
> reason why that shouldn't work?

Ouch! I mingled the two cases
together. RequestNamedLWLockTranche'd tranches are registerd in
the main tranche array.

Sorry for the noise and thanks a lot for kindly answering me.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center