trafodion-codereview mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From zellerh <...@git.apache.org>
Subject [GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Date Tue, 03 Apr 2018 17:54:55 GMT
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.


---

Mime
View raw message