[TRAFODION-2853] memory leak of ComDiagsArea in CmpContext heap of mxosrvr #1470
Conversation
…osrvr The ComDiagsArea is allocated in many places and from different heaps in Trafodion, making it difficult to detect the source of the leak. Hence, a different approach is taken to fix this issue. Currently, ComDiagsArea is allocated in many places unconditionally even when SQL statement completes execution without any error or warnings. Then it is deallocated. Changed this strategy and allocate ComDiagsArea only when there is an error or warning while compiling or executing the SQL statement. This should help the product in two ways 1) To reduce the pathlength. The smaller query execution would benefit the most by chopping of few more microseconds. 2) Reduce the memory growth due to leaked ComDiagsArea
…osrvr Fixes for the regression failures seen with b97982c This includes the change to report the error at the time of compilation for invoke, showddl commands. Earlier errors were ignored during prepare time and reported only at the time of execute for these commands
|
Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2473/ |
|
+1 |
|
Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/2473/ |
…osrvr Fix for the regression failures seen with commit 07f41dd
|
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2476/ |
|
Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/2476/ |
| // tbd - figure a way retry registration later, as the query progresses. | ||
| if (diagsArea != NULL) | ||
| NegateAllErrors(diagsArea); | ||
| diagsArea->negateErrors(fromCond); |
There was a problem hiding this comment.
The method's caller worries about deallocating the diagsArea in this code path, right?
There was a problem hiding this comment.
Yes. The caller is responsible to deallocate it
| @@ -837,7 +837,7 @@ class ComDiagsArea : public IpcMessageObj { | |||
|
|
|||
| // These members provide set and get operations on the data | |||
| // of a ComDiagsArea that is defined in ANSI table 21, in subclause | |||
| // 18.1. See also, ``Creating Errors Korrectly.'' | |||
| // 18.1. See also, ``Creating Errors Correctly.'' | |||
There was a problem hiding this comment.
I think "Creating Errors Korrectly" is a reference to a paper Bill Rassieur wrote way back in HP days. (And yes he intentionally mis-spelled "Correctly".) I don't know if a copy is up on the Trafodion web site or not. Might be nice to put it there if not.
| if (rc == 0) | ||
| return 0; | ||
| else | ||
| return diagsCondCount; |
There was a problem hiding this comment.
This is bizzarre. So if SQL_EXEC_GetDiagnosticsStmtInfo2 returns an error, we return diagsCondCount?
There was a problem hiding this comment.
I believe the return value is context()->diags()->mainSQLCode() which would be 100 from the previous CLI call. Yes. It is bizarre. This change was done to get the warnings when some regressions failed.
There was a problem hiding this comment.
I just realized that I misread the function SQLCLI_ReturnCode. It looks like diagsCondCount is uninitialized and hence junk value was returned for getDiagsCondCount that enabled us to return warnings. Basically two issues made this function to behave as expected most of the time. I will be fixing it soon
|
+1 on the third commit (already gave my +1 for the rest) |
The ComDiagsArea is allocated in many places and from different heaps in Trafodion, making it difficult to detect the source of the leak. Hence, a different approach is taken to fix this issue.
Currently, ComDiagsArea is allocated in many places unconditionally even when SQL statement completes execution without any error or warnings. Then it is deallocated. Changed this strategy and allocate ComDiagsArea only when there is an error or warning while compiling or executing the SQL statement.
This should help the product in two ways
1) To reduce the path length. The smaller query execution would benefit the most by chopping of few more microseconds.
2) Reduce the memory growth due to leaked ComDiagsArea