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_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
>From e2c4619df6c51dd5b71630de7467b332f7449fc5 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyot...@lab.ntt.co.jp>
Date: Fri, 3 Mar 2017 15:47:19 +0900
Subject: [PATCH] Remove NamedLWLockTrancheArray
This patch fixes two pattern of server crash of postgresql compled
with --with-dtarce.
One occurs when an extension forgets to call LWLockRegisterTranche for
the tranche id given by LWLockNewTrancheId. For the case, trace point
code of lwlocks looks up to LWLockTrancheArray for tranche names but
it can be outside of valid(mapped) memory. This seems quite common
mistake but hardly detectable by ordinary tests. Range check of
tranche_id in LWLockInitialize can detect the case.
The other occurs when an extension uses RequestNamedfLWLockTranche and
GetNamedLWLockTrasnche. In this case the tranches are stored in
NamedLWLockTrancheArray separatley from LWLockTrancheArray. This also
may cause a crash when a tracepoint is performed. This patch merges
all tranches into LWLockTrancheArray and remove NameLWLockTrancheArray.
---
src/backend/storage/lmgr/lwlock.c | 89 ++++++++++++++++++++++-----------------
src/include/storage/lwlock.h | 8 ----
2 files changed, 51 insertions(+), 46 deletions(-)
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index ab81d94..4d2a1fc 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -153,8 +153,6 @@ NamedLWLockTrancheRequest *NamedLWLockTrancheRequestArray = NULL;
static int NamedLWLockTrancheRequestsAllocated = 0;
int NamedLWLockTrancheRequests = 0;
-NamedLWLockTranche *NamedLWLockTrancheArray = NULL;
-
static bool lock_named_request_allowed = true;
static void InitializeLWLocks(void);
@@ -360,9 +358,6 @@ LWLockShmemSize(void)
/* Space for dynamic allocation counter, plus room for alignment. */
size = add_size(size, sizeof(int) + LWLOCK_PADDED_SIZE);
- /* space for named tranches. */
- size = add_size(size, mul_size(NamedLWLockTrancheRequests, sizeof(NamedLWLockTranche)));
-
/* space for name of each tranche. */
for (i = 0; i < NamedLWLockTrancheRequests; i++)
size = add_size(size, strlen(NamedLWLockTrancheRequestArray[i].tranche_name) + 1);
@@ -410,13 +405,16 @@ CreateLWLocks(void)
*/
LWLockCounter = (int *) ((char *) MainLWLockArray - sizeof(int));
*LWLockCounter = LWTRANCHE_FIRST_USER_DEFINED;
-
- /* Initialize all LWLocks */
- InitializeLWLocks();
}
- /* Register all LWLock tranches */
+ /* Register all LWLock tranches on local memory */
RegisterLWLockTranches();
+
+ /* Initialize all LWLocks */
+ if (!IsUnderPostmaster)
+ InitializeLWLocks();
+
+
}
/*
@@ -425,7 +423,6 @@ CreateLWLocks(void)
static void
InitializeLWLocks(void)
{
- int numNamedLocks = NumLWLocksByNamedTranches();
int id;
int i;
int j;
@@ -451,35 +448,22 @@ InitializeLWLocks(void)
for (id = 0; id < NUM_PREDICATELOCK_PARTITIONS; id++, lock++)
LWLockInitialize(&lock->lock, LWTRANCHE_PREDICATE_LOCK_MANAGER);
- /* Initialize named tranches. */
+ /*
+ * Initialize LWLokcs in user tranches defined using
+ * RequestNamedLWLockTranche
+ */
if (NamedLWLockTrancheRequests > 0)
{
- char *trancheNames;
+ LWLockPadded *lock = &MainLWLockArray[NUM_FIXED_LWLOCKS];
- NamedLWLockTrancheArray = (NamedLWLockTranche *)
- &MainLWLockArray[NUM_FIXED_LWLOCKS + numNamedLocks];
-
- trancheNames = (char *) NamedLWLockTrancheArray +
- (NamedLWLockTrancheRequests * sizeof(NamedLWLockTranche));
- lock = &MainLWLockArray[NUM_FIXED_LWLOCKS];
-
- for (i = 0; i < NamedLWLockTrancheRequests; i++)
+ for (i = 0 ; i < NamedLWLockTrancheRequests ; i++)
{
- NamedLWLockTrancheRequest *request;
- NamedLWLockTranche *tranche;
- char *name;
+ NamedLWLockTrancheRequest *request =
+ &NamedLWLockTrancheRequestArray[i];
+ int tranche_id = LWTRANCHE_FIRST_USER_DEFINED + i;
- request = &NamedLWLockTrancheRequestArray[i];
- tranche = &NamedLWLockTrancheArray[i];
-
- name = trancheNames;
- trancheNames += strlen(request->tranche_name) + 1;
- strcpy(name, request->tranche_name);
- tranche->trancheId = LWLockNewTrancheId();
- tranche->trancheName = name;
-
- for (j = 0; j < request->num_lwlocks; j++, lock++)
- LWLockInitialize(&lock->lock, tranche->trancheId);
+ for (j = 0 ; j < request->num_lwlocks ; j++, lock++)
+ LWLockInitialize(&lock->lock, tranche_id);
}
}
}
@@ -495,6 +479,10 @@ RegisterLWLockTranches(void)
if (LWLockTrancheArray == NULL)
{
LWLockTranchesAllocated = 64;
+ while(LWTRANCHE_FIRST_USER_DEFINED + NamedLWLockTrancheRequests
+ > LWLockTranchesAllocated)
+ LWLockTranchesAllocated *= 2;
+
LWLockTrancheArray = (char **)
MemoryContextAllocZero(TopMemoryContext,
LWLockTranchesAllocated * sizeof(char *));
@@ -511,10 +499,29 @@ RegisterLWLockTranches(void)
LWLockRegisterTranche(LWTRANCHE_PARALLEL_QUERY_DSA,
"parallel_query_dsa");
- /* Register named tranches. */
- for (i = 0; i < NamedLWLockTrancheRequests; i++)
- LWLockRegisterTranche(NamedLWLockTrancheArray[i].trancheId,
- NamedLWLockTrancheArray[i].trancheName);
+ if (NamedLWLockTrancheRequests > 0)
+ {
+ char *trancheNames =
+ (char *) &MainLWLockArray[NUM_FIXED_LWLOCKS +
+ NumLWLocksByNamedTranches()];
+
+ for (i = 0; i < NamedLWLockTrancheRequests; i++)
+ {
+ char *name;
+ int tranche_id;
+ NamedLWLockTrancheRequest *request;
+
+ tranche_id = LWLockNewTrancheId();
+ request = &NamedLWLockTrancheRequestArray[i];
+
+ /* Copy the name in request to shared memory */
+ name = trancheNames;
+ trancheNames += strlen(request->tranche_name) + 1;
+ strcpy(name, request->tranche_name);
+
+ LWLockRegisterTranche(tranche_id, name);
+ }
+ }
}
/*
@@ -665,6 +672,12 @@ RequestNamedLWLockTranche(const char *tranche_name, int num_lwlocks)
void
LWLockInitialize(LWLock *lock, int tranche_id)
{
+ if (tranche_id >= LWTRANCHE_FIRST_USER_DEFINED &&
+ tranche_id >= LWLockTranchesAllocated)
+ ereport(ERROR,
+ (errmsg ("tranche for this lock not available"),
+ errdetail("LWLockRegisterTranche for this tranche required")));
+
pg_atomic_init_u32(&lock->state, LW_FLAG_RELEASE_OK);
#ifdef LOCK_DEBUG
pg_atomic_init_u32(&lock->nwaiters, 0);
diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index 8bd93c3..4645414 100644
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -90,14 +90,6 @@ typedef union LWLockMinimallyPadded
extern PGDLLIMPORT LWLockPadded *MainLWLockArray;
extern char *MainLWLockNames[];
-/* struct for storing named tranche information */
-typedef struct NamedLWLockTranche
-{
- int trancheId;
- char *trancheName;
-} NamedLWLockTranche;
-
-extern PGDLLIMPORT NamedLWLockTranche *NamedLWLockTrancheArray;
extern PGDLLIMPORT int NamedLWLockTrancheRequests;
/* Names for fixed lwlocks */
--
2.9.2
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
