Repository: trafodion Updated Branches: refs/heads/master 7c0ab57e7 -> d7644b9dc
[TRAFODION-2727] Memory leak in the compiler part of the code in Trafodion It was found that CmpMessageReplyCode was not getting deleted at all times. But, the handling of CmpMessageDescribe messages in the compiler code has some mysterious issue that corrupts the memory causing unpredictable behavior when CmpMessageReplyCode is deleted. To circumvent this problem, the CmpStatement heap is used for embedded compiler to allocate CmpMessageReplyCode and the Context heap is used in case of standalone compiler. The handling of CmpMessageDescribe will continue to leak memory in standalone compiler. Project: http://git-wip-us.apache.org/repos/asf/trafodion/repo Commit: http://git-wip-us.apache.org/repos/asf/trafodion/commit/52703c64 Tree: http://git-wip-us.apache.org/repos/asf/trafodion/tree/52703c64 Diff: http://git-wip-us.apache.org/repos/asf/trafodion/diff/52703c64 Branch: refs/heads/master Commit: 52703c64dbca6af7f7ad17d7e0efe15d7f6afd0c Parents: 087af70 Author: selvaganesang <[email protected]> Authored: Fri Feb 16 02:03:46 2018 +0000 Committer: selvaganesang <[email protected]> Committed: Fri Feb 16 02:03:46 2018 +0000 ---------------------------------------------------------------------- core/sql/arkcmp/CmpConnection.cpp | 8 +++--- core/sql/arkcmp/CmpStatement.cpp | 49 +++++++++++++++++----------------- core/sql/arkcmp/CmpStatement.h | 8 ++++-- core/sql/optimizer/Rule.cpp | 2 ++ 4 files changed, 37 insertions(+), 30 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/trafodion/blob/52703c64/core/sql/arkcmp/CmpConnection.cpp ---------------------------------------------------------------------- diff --git a/core/sql/arkcmp/CmpConnection.cpp b/core/sql/arkcmp/CmpConnection.cpp index 0acf0d8..aa72355 100644 --- a/core/sql/arkcmp/CmpConnection.cpp +++ b/core/sql/arkcmp/CmpConnection.cpp @@ -349,7 +349,7 @@ void ExCmpMessage::actOnReceive(IpcConnection* ) // The number of NAMemory objects that reside in SYSTEM_MEMORY // is currently restricted to one at a time--see explanation in // NAMemory.h. - cmpStatement = new CTXTHEAP CmpStatement(cmpContext_, 0, NAMemory::SYSTEM_MEMORY); + cmpStatement = new CTXTHEAP CmpStatement(cmpContext_); CmpMessageSQLText sqltext(NULL,0,CTXTHEAP,SQLCHARSETCODE_UNKNOWN, (CmpMessageObj::MessageTypeEnum)typ); receiveAndSetUp(this, sqltext); @@ -388,7 +388,7 @@ void ExCmpMessage::actOnReceive(IpcConnection* ) case (CmpMessageObj::DDL) : { - cmpStatement = new CTXTHEAP CmpStatement(cmpContext_, 0, NAMemory::SYSTEM_MEMORY); + cmpStatement = new CTXTHEAP CmpStatement(cmpContext_); CmpMessageDDL statement(NULL, 0, CTXTHEAP); receiveAndSetUp(this, statement); cmpStatement->process(statement); @@ -397,7 +397,7 @@ void ExCmpMessage::actOnReceive(IpcConnection* ) case (CmpMessageObj::DESCRIBE) : { - cmpStatement = new CTXTHEAP CmpStatement(cmpContext_, 0, NAMemory::SYSTEM_MEMORY); + cmpStatement = new CTXTHEAP CmpStatement(cmpContext_); CmpMessageDescribe statement(NULL, 0, CTXTHEAP); receiveAndSetUp(this, statement); cmpStatement->process(statement); @@ -433,7 +433,7 @@ void ExCmpMessage::actOnReceive(IpcConnection* ) case (CmpMessageObj::DDL_WITH_STATUS) : { - cmpStatement = new CTXTHEAP CmpStatement(cmpContext_, 0, NAMemory::SYSTEM_MEMORY); + cmpStatement = new CTXTHEAP CmpStatement(cmpContext_); CmpMessageDDLwithStatus statement(NULL, 0, CTXTHEAP); receiveAndSetUp(this, statement); cmpStatement->process(statement); http://git-wip-us.apache.org/repos/asf/trafodion/blob/52703c64/core/sql/arkcmp/CmpStatement.cpp ---------------------------------------------------------------------- diff --git a/core/sql/arkcmp/CmpStatement.cpp b/core/sql/arkcmp/CmpStatement.cpp index 69e1f45..f66baae 100644 --- a/core/sql/arkcmp/CmpStatement.cpp +++ b/core/sql/arkcmp/CmpStatement.cpp @@ -136,12 +136,12 @@ CmpStatement::error(Lng32 no, const char* s) CmpStatement::CmpStatement(CmpContext* context, - CollHeap* outHeap, - NAMemory::NAMemoryType memoryType) + CollHeap* outHeap) : parserStmtLiteralList_(outHeap) { exceptionRaised_ = FALSE; - reply_ = 0; + reply_ = NULL; + bound_ = NULL; context_ = context; storedProc_ = 0; prvCmpStatement_ = 0; @@ -171,15 +171,17 @@ CmpStatement::CmpStatement(CmpContext* context, memLimit); heap_->setErrorCallback(&CmpErrLog::CmpErrLogCallback); } + + // Embedded arkcmp reply is consumed by the caller before the CmpStatement + // is deleted, hence use CmpStatement Heap itself to avoid any leaks - // The default output heap will be the context heap, because the - // CmpMessageReply object might last after the CmpStatement goes - // out of scope. - outHeap_ = outHeap ? outHeap : context_ ? context_->heap() : 0; + if (context_->isEmbeddedArkcmp()) + outHeap_ = heap_; + else + outHeap_ = context_->heap(); context->setStatement(this); - compStats_ = new (heap_) CompilationStats(); optGlobals_ = new (heap_) OptGlobals(heap_); @@ -210,9 +212,19 @@ CmpStatement::~CmpStatement() // to end the interface. delete storedProc_; - if (reply_) + if (reply_ != NULL) reply_->decrRefCount(); +/* + // At times, this delete can cause corruption in the heap + // Hence, it is commented out for now - Selva + // To miminze the leak from this the heap_ that used for this + // objects comes from CmpStatement Heap in case of embedded arkcmp, + // and from CmpContext Heap in case of standalone arkcmp. + if (bound_ != NULL) + bound_->decrRefCount(); +*/ + // GLOBAL_EMPTY_INPUT_LOGPROP points to an EstLogProp object in the heap. // Because it is a SharedPtr, it must be set to NULL before the statement // heap is destroyed. However, there are cases where a temporary @@ -760,9 +772,6 @@ CmpStatement::process (const CmpMessageDDL& statement) QueryText qText(sqlStr, inputCS); - CmpMessageReplyCode - *bound = new(outHeap_) CmpMessageReplyCode(outHeap_, statement.id(), 0, 0, outHeap_); - // CmpMain cmpmain; Set_SqlParser_Flags(DELAYED_RESET); // sqlcompCleanup resets for us Parser parser(CmpCommon::context()); @@ -862,9 +871,6 @@ short CmpStatement::getDDLExprAndNode(char * sqlStr, Lng32 inputCS, QueryText qText(sqlStr, inputCS); - // CmpMessageReplyCode - // *bound = new(outHeap_) CmpMessageReplyCode(outHeap_, statement.id(), 0, 0, outHeap_); - Set_SqlParser_Flags(DELAYED_RESET); // sqlcompCleanup resets for us Parser parser(CmpCommon::context()); BindWA bindWA(ActiveSchemaDB(), CmpCommon::context(), TRUE); @@ -1033,13 +1039,8 @@ CmpStatement::ReturnStatus CmpStatement::process (const CmpMessageDescribe& statement) { ReturnStatus ret = CmpStatement_SUCCESS; - // There will be memory leak handling CmpMessageReplyCode *bound this way. - // The correct way should be making bound a local variable with statementHeap passed in. - // But Matt has tried it, there seem to be some memory problems show up later, - // so in the future, when time allows in the memory cleanup stage, this code should be - // revisited. - CmpMessageReplyCode - *bound = new(outHeap_) CmpMessageReplyCode(outHeap_, statement.id(), 0, 0, outHeap_); + + bound_ = new(outHeap_) CmpMessageReplyCode(outHeap_, statement.id(), 0, 0, outHeap_); reply_ = new(outHeap_) CmpMessageReplyCode(outHeap_, statement.id(), 0, 0, outHeap_); // A pointer to user SQL query is stored in CmpStatement; if an exception is @@ -1067,11 +1068,11 @@ CmpStatement::process (const CmpMessageDescribe& statement) // pass this (casting to RelExpr, which it really is) to CmpDescribe CmpMain cmpmain; if (cmpmain.sqlcomp(qText, 0, //IN - &bound->data(), &bound->size(), bound->outHeap(), //OUT + &bound_->data(), &bound_->size(), bound_->outHeap(), //OUT CmpMain::BIND) //IN || CmpDescribe(statement.data(), //IN - (RelExpr*)bound->data(), //IN + (RelExpr*)bound_->data(), //IN reply_->data(), reply_->size(), reply_->outHeap())) //OUT { error(arkcmpErrorNoDiags, statement.data()); http://git-wip-us.apache.org/repos/asf/trafodion/blob/52703c64/core/sql/arkcmp/CmpStatement.h ---------------------------------------------------------------------- diff --git a/core/sql/arkcmp/CmpStatement.h b/core/sql/arkcmp/CmpStatement.h index dc0767a..097d0cf 100644 --- a/core/sql/arkcmp/CmpStatement.h +++ b/core/sql/arkcmp/CmpStatement.h @@ -86,8 +86,7 @@ public: // memoryType parameter is ignored CmpStatement( CmpContext*, - NAMemory* outHeap = 0, - NAMemory::NAMemoryType memoryType=NAMemory::DERIVED_MEMORY); + NAMemory *outHeap = NULL); // requests process ReturnStatus process(const CmpMessageObj&); @@ -254,6 +253,11 @@ protected: // The reply to be sent back to executor after processing the request in CmpStatement CmpMessageReply* reply_; + // The result of Compile for statements like invoke, get tables, show stats + // that calls CmpDescribe internally immediately after compilation. + + CmpMessageReply *bound_; + // The flag to record whether exception has been raised in the // statement compilation/execution. This is used to clean up properly once the // exception is raised ( especially when longjmp occurred ) http://git-wip-us.apache.org/repos/asf/trafodion/blob/52703c64/core/sql/optimizer/Rule.cpp ---------------------------------------------------------------------- diff --git a/core/sql/optimizer/Rule.cpp b/core/sql/optimizer/Rule.cpp index d418fd3..0be2941 100644 --- a/core/sql/optimizer/Rule.cpp +++ b/core/sql/optimizer/Rule.cpp @@ -394,6 +394,8 @@ RuleSet::~RuleSet() { for (Lng32 i = 0; i < (Lng32)allRules_.entries(); i++) delete allRules_[i]; + for (Lng32 i = 0; i < (Lng32)passNRules_.entries(); i++) + delete passNRules_[i]; } void RuleSet::insert(Rule * r)
