You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by selvaganesang <gi...@git.apache.org> on 2017/10/26 04:56:11 UTC
[GitHub] incubator-trafodion pull request #1277: [TRAFODION-2783] jdbc_test_cdh fails...
GitHub user selvaganesang opened a pull request:
https://github.com/apache/incubator-trafodion/pull/1277
[TRAFODION-2783] jdbc_test_cdh fails at times with type 2 JDBC driver…
… dumping core
In case of Type 2 JDBC driver, the Trafodion SQL engine is a library that is dynamic
loaded into the process. Initialization of C++ static objects in the dynamic loaded libraries
are supposed to be done before dlopen returns. But the behavior seems to be nondeterministic when
there are multiple threads or when there are dependent static objects (An static object expects
another to be initialized before it). I think, the order of the initialization is not guaranteed
by the standard.
Refactored the code to initialize static object CharInfo::builtinCollationDB_ as part of the first
CLI call in a thread safe manner.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/selvaganesang/incubator-trafodion static_variable
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-trafodion/pull/1277.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1277
----
commit 9a8a7c39fecff583d2988c76fb443620ee64df24
Author: selvaganesang <se...@esgyn.com>
Date: 2017-10-26T04:47:16Z
[TRAFODION-2783] jdbc_test_cdh fails at times with type 2 JDBC driver dumping core
In case of Type 2 JDBC driver, the Trafodion SQL engine is a library that is dynamic
loaded into the process. Initialization of C++ static objects in the dynamic loaded libraries
are supposed to be done before dlopen returns. But the behavior seems to be nondeterministic when
there are multiple threads or when there are dependent static objects (An static object expects
another to be initialized before it). I think, the order of the initialization is not guaranteed
by the standard.
Refactored the code to initialize static object CharInfo::builtinCollationDB_ as part of the first
CLI call in a thread safe manner.
----
---
[GitHub] incubator-trafodion pull request #1277: [TRAFODION-2783] jdbc_test_cdh fails...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1277#discussion_r147222686
--- Diff: core/sql/common/charinfo.h ---
@@ -260,11 +260,14 @@ class CharInfo
Int32 sourceLenInBytes,
CharSet targetCS);
+ static void initBuiltinCollationDB();
+
+
private:
friend class CollationDB; // needs to access builtinCDB_
static const char* const localeCharSet_;
- static const CollationDB builtinCollationDB_;
+ static const CollationDB *builtinCollationDB_;
--- End diff --
Suggest initializing to NULL. So if you get a thread race condition, you'll get better behavior if one thread accessing this pointer beats the other thread that is initializing it.
---
[GitHub] incubator-trafodion pull request #1277: [TRAFODION-2783] jdbc_test_cdh fails...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-trafodion/pull/1277
---
[GitHub] incubator-trafodion pull request #1277: [TRAFODION-2783] jdbc_test_cdh fails...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1277#discussion_r147227161
--- Diff: core/sql/common/charinfo.h ---
@@ -260,11 +260,14 @@ class CharInfo
Int32 sourceLenInBytes,
CharSet targetCS);
+ static void initBuiltinCollationDB();
+
+
private:
friend class CollationDB; // needs to access builtinCDB_
static const char* const localeCharSet_;
- static const CollationDB builtinCollationDB_;
+ static const CollationDB *builtinCollationDB_;
--- End diff --
Oh, sorry; this is the declaration rather than the definition. Ignore my comment.
---
[GitHub] incubator-trafodion pull request #1277: [TRAFODION-2783] jdbc_test_cdh fails...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1277#discussion_r147250524
--- Diff: core/sql/common/charinfo.cpp ---
@@ -693,3 +696,9 @@ Int32 CharInfo::getMaxConvertedLenInBytes(CharSet sourceCS,
return ((sourceLenInBytes/minBytesPerChar(sourceCS)) *
maxBytesPerChar(targetCS));
}
+
+void CharInfo::initBuiltinCollationDB()
+{
+ builtinCollationDB_ = new CollationDB(NULL, mapCOArray, SIZEOF_CO);
--- End diff --
Yes. C++ prologue ensures that it is initialized correctly. In master process, it will be initialized before dlOpen and in other processes it should be done before main is called.
---
[GitHub] incubator-trafodion pull request #1277: [TRAFODION-2783] jdbc_test_cdh fails...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1277#discussion_r147223897
--- Diff: core/sql/common/charinfo.cpp ---
@@ -693,3 +696,9 @@ Int32 CharInfo::getMaxConvertedLenInBytes(CharSet sourceCS,
return ((sourceLenInBytes/minBytesPerChar(sourceCS)) *
maxBytesPerChar(targetCS));
}
+
+void CharInfo::initBuiltinCollationDB()
+{
+ builtinCollationDB_ = new CollationDB(NULL, mapCOArray, SIZEOF_CO);
--- End diff --
Is mapCOArray another global object? If it is const it is OK. If not, how do we know that it has been constructed already?
---
[GitHub] incubator-trafodion pull request #1277: [TRAFODION-2783] jdbc_test_cdh fails...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1277#discussion_r147223670
--- Diff: core/sql/common/charinfo.cpp ---
@@ -529,8 +530,10 @@ static const CollationInfo mapCOArray[] = {
CollationInfo(NULL, CharInfo::UNKNOWN_COLLATION, SQLCOLLATIONSTRING_UNKNOWN,
STATIC_NEG)
};
-static const size_t SIZEOF_CO = sizeof(mapCOArray)/sizeof(CollationInfo);
-const CollationDB CharInfo::builtinCollationDB_(NULL, mapCOArray, SIZEOF_CO);
+
+#define SIZEOF_CO (sizeof(mapCOArray)/sizeof(CollationInfo))
+
+const CollationDB *CharInfo::builtinCollationDB_;
--- End diff --
Suggest initializing this global pointer to null, so you get better behavior in race conditions.
---
[GitHub] incubator-trafodion pull request #1277: [TRAFODION-2783] jdbc_test_cdh fails...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1277#discussion_r147250690
--- Diff: core/sql/common/charinfo.cpp ---
@@ -529,8 +530,10 @@ static const CollationInfo mapCOArray[] = {
CollationInfo(NULL, CharInfo::UNKNOWN_COLLATION, SQLCOLLATIONSTRING_UNKNOWN,
STATIC_NEG)
};
-static const size_t SIZEOF_CO = sizeof(mapCOArray)/sizeof(CollationInfo);
-const CollationDB CharInfo::builtinCollationDB_(NULL, mapCOArray, SIZEOF_CO);
+
+#define SIZEOF_CO (sizeof(mapCOArray)/sizeof(CollationInfo))
+
+const CollationDB *CharInfo::builtinCollationDB_;
--- End diff --
It is required in the rework and it is initialized to NULL
---
[GitHub] incubator-trafodion pull request #1277: [TRAFODION-2783] jdbc_test_cdh fails...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1277#discussion_r147251925
--- Diff: core/sql/common/charinfo.cpp ---
@@ -363,21 +363,22 @@ void CollationInfo::display() const
CollationDB::CollationDB(CollHeap *h)
: CollationDBSupertype(h), heap_(h), refreshNeeded_(TRUE)
{
- if (this == &CharInfo::builtinCollationDB_) return;
+ if (this == CharInfo::builtinCollationDB_) return;
--- End diff --
CharInfo::builtinCollationDB_ is a singleton object. It looks like there could other CollationDB objects, but it will have all the data in the builtinCollationDB object too
---
[GitHub] incubator-trafodion pull request #1277: [TRAFODION-2783] jdbc_test_cdh fails...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1277#discussion_r147223503
--- Diff: core/sql/common/charinfo.cpp ---
@@ -363,21 +363,22 @@ void CollationInfo::display() const
CollationDB::CollationDB(CollHeap *h)
: CollationDBSupertype(h), heap_(h), refreshNeeded_(TRUE)
{
- if (this == &CharInfo::builtinCollationDB_) return;
+ if (this == CharInfo::builtinCollationDB_) return;
--- End diff --
I'm confused. Is CollationDB a singleton object? Or is there one global version, and other parts of the code might create their own version?
---