Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r178900839
--- Diff: core/sql/executor/ExExeUtilExplain.cpp ---
@@ -237,8 +237,7 @@ short ExExeUtilDisplayExplainTcb::work()
executeImmediate("control session 'EXPLAIN' 'ON';");
if (retcode < 0)
{
- cliInterface()->retrieveSQLDiagnostics(getDiagsArea());
-
+
setDiagsArea(cliInterface()->allocAndRetrieveSQLDiagnostics(getDiagsArea()));
--- End diff --
From what I can tell, ExExeUtilTcb::getDiagsArea() returns a ComDiagsArea
*. The compiler then makes a reference of this temporary value and passes it to
allocAndRetrieveSQLDiagnostics(). That method may allocate a new diags area. If
we then encounter an error when merging the diags area in
allocAndRetrieveSQLDiagnostics(), it will return NULL and the allocated diags
area is leaked. See also comment above. I think it would be better to create an
ExExeUtilTcb member function like this:
```
void retrieveCliDiags() {
cliInterface_->allocAndRetrieveSQLDiagnistics(diagsArea_); }
```
Then, the line here could be replace by a call to retrieveCliDiags() and we
wouldn't have to deal with all the potential complications mentioned in this
and a previous comment.
---
