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 2018/04/03 01:18:42 UTC
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
GitHub user selvaganesang opened a pull request:
https://github.com/apache/trafodion/pull/1504
[TRAFODION-3009] Streamline error handling in Executor utility commands
ComDiagsArea is now allocated only when there are warnings or error in
all the utility commands except load. In case of load, the ComDiagsArea
is allocated in advance to report error rows count.
This requires all the executor utility commands to use a new version of
ExRaiseSqlError to populate diagnostics area.
[TRAFODION-3017] Simplify the hive client access in Trafodion
Hive Client functions are now moved to a new file HiveClient_JNI.h and
HiveClient_JNI.cpp. Most of the HiveClient functions are static functions
allowing to use HiveClient in Trafodion with ease.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/selvaganesang/trafodion trafodion-3009_1
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1504.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 #1504
----
commit 1f44166c843fd1a6e8b325a2fcbe0cf8158e099b
Author: selvaganesang <se...@...>
Date: 2018-03-29T00:13:15Z
[TRAFODION-3009] Streamline error handling in Executor utility commands
ComDiagsArea is now allocated only when there are warnings or error in
all the utility commands except load. In case of load, the ComDiagsArea
is allocated in advance to report error rows count.
This requires all the executor utility commands to use a new version of
ExRaiseSqlError to populate diagnostics area.
[TRAFODION-3017] Simplify the hive client access in Trafodion
Hive Client functions are now moved to a new file HiveClient_JNI.h and
HiveClient_JNI.cpp. Most of the HiveClient functions are static functions
allowing to use HiveClient in Trafodion with ease.
----
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r179548853
--- Diff: core/sql/sqlcomp/CmpSeabaseDDLschema.cpp ---
@@ -410,7 +410,8 @@ Int64 schemaUID = getObjectTypeandOwner(&cliInterface,
if (schemaUID < 0)
{
*CmpCommon::diags() << DgSqlCode(-CAT_SCHEMA_DOES_NOT_EXIST_ERROR)
- << DgSchemaName(catalogName + "." + schemaName);
+ << DgString0(catalogName)
--- End diff --
See my earlier comments
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafodion/pull/1504
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r178901892
--- Diff: core/sql/executor/ExExeUtilCli.cpp ---
@@ -2091,27 +2084,57 @@ Lng32 ExeCliInterface::deleteContext(char* contextHandle) // in buf contains con
Lng32 ExeCliInterface::retrieveSQLDiagnostics(ComDiagsArea *toDiags)
{
Lng32 retcode;
-
+ ex_assert(toDiags != NULL, "ComDiagsArea is null");
if (diagsArea_ != NULL)
{
diagsArea_->clear();
diagsArea_->deAllocate();
}
+ retcode = SQL_EXEC_MergeDiagnostics_Internal(*toDiags);
+ SQL_EXEC_ClearDiagnostics(NULL);
+ return retcode;
+}
- if (toDiags != NULL)
- {
- retcode = SQL_EXEC_MergeDiagnostics_Internal(*toDiags);
- SQL_EXEC_ClearDiagnostics(NULL);
- }
- else
+
+ComDiagsArea *ExeCliInterface::allocAndRetrieveSQLDiagnostics(ComDiagsArea *&toDiags)
+{
+ Lng32 retcode;
+
+ if (diagsArea_ != NULL)
{
- diagsArea_ = ComDiagsArea::allocate(heap_);
- retcode = SQL_EXEC_MergeDiagnostics_Internal(*diagsArea_);
+ diagsArea_->clear();
+ diagsArea_->deAllocate();
}
+ if (toDiags == NULL)
+ toDiags = ComDiagsArea::allocate(heap_);
- return retcode;
+ retcode = SQL_EXEC_MergeDiagnostics_Internal(*toDiags);
+ SQL_EXEC_ClearDiagnostics(NULL);
+
+ if (retcode == 0)
+ return toDiags;
+ else
+ return NULL;
--- End diff --
See also below for a possible leak. Instead of returning NULL, it may be better to add a condition to the allocated diags area. That would make it easier for the caller to check (no check needed in most cases).
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r178971037
--- Diff: core/sql/executor/ExExeUtilGet.cpp ---
@@ -1818,8 +1813,9 @@ short ExExeUtilGetMetadataInfoTcb::work()
{
if (!CmpCommon::context()->isAuthorizationEnabled())
{
- ComDiagsArea * diags = getDiagsArea();
- *diags << DgSqlCode(-CAT_AUTHORIZATION_NOT_ENABLED);
+ ComDiagsArea * diagsArea = getDiagsArea();
+ ExRaiseSqlError(getHeap(), &diagsArea, -CAT_AUTHORIZATION_NOT_ENABLED);
+ setDiagsArea(diagsArea);
--- End diff --
Sorry, I'm not sure I understand. What I am suggesting doesn't require you to change any other cases, it just makes this case (and the many repetitions of it in your commit) simpler.
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r178926825
--- Diff: core/sql/executor/ExExeUtilCli.cpp ---
@@ -2091,27 +2084,57 @@ Lng32 ExeCliInterface::deleteContext(char* contextHandle) // in buf contains con
Lng32 ExeCliInterface::retrieveSQLDiagnostics(ComDiagsArea *toDiags)
{
Lng32 retcode;
-
+ ex_assert(toDiags != NULL, "ComDiagsArea is null");
if (diagsArea_ != NULL)
{
diagsArea_->clear();
diagsArea_->deAllocate();
}
+ retcode = SQL_EXEC_MergeDiagnostics_Internal(*toDiags);
+ SQL_EXEC_ClearDiagnostics(NULL);
+ return retcode;
+}
- if (toDiags != NULL)
- {
- retcode = SQL_EXEC_MergeDiagnostics_Internal(*toDiags);
- SQL_EXEC_ClearDiagnostics(NULL);
- }
- else
+
+ComDiagsArea *ExeCliInterface::allocAndRetrieveSQLDiagnostics(ComDiagsArea *&toDiags)
+{
+ Lng32 retcode;
+
+ if (diagsArea_ != NULL)
{
- diagsArea_ = ComDiagsArea::allocate(heap_);
- retcode = SQL_EXEC_MergeDiagnostics_Internal(*diagsArea_);
+ diagsArea_->clear();
+ diagsArea_->deAllocate();
}
+ if (toDiags == NULL)
+ toDiags = ComDiagsArea::allocate(heap_);
- return retcode;
+ retcode = SQL_EXEC_MergeDiagnostics_Internal(*toDiags);
+ SQL_EXEC_ClearDiagnostics(NULL);
+
+ if (retcode == 0)
+ return toDiags;
+ else
+ return NULL;
}
+void ExeCliInterface::retrieveOrSaveSQLDiagnostics(Lng32 cliRetcode, ComDiagsArea *diagsArea)
--- End diff --
This function is not used. I created this function to avoid creating ComDiagsArea in advance for load operations. When I attempted to refactor the retrieveSQLDiagnostics and allocAndRetrieveSQLDiagnostics, it opened up more issues. I will remove this function
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r179530143
--- Diff: core/sql/bin/SqlciErrors.txt ---
@@ -2,7 +2,7 @@
1000 42000 99999 BEGINNER INFRM LOGONLY A syntax error occurred.
1001 ZZZZZ 99999 ADVANCED CRTCL DIALOUT An internal error occurred in module $0~String0 on line $1~Int0. Details($2~String1).
1002 ZZZZZ 99999 BEGINNER MAJOR DBADMIN Catalog $0~CatalogName does not exist.
-1003 ZZZZZ 99999 BEGINNER MINOR DBADMIN Schema $0~SchemaName does not exist.
+1003 ZZZZZ 99999 BEGINNER MINOR DBADMIN Schema $0~String0.$1~String1 does not exist.
--- End diff --
I'm guessing String0 is CatalogName and String1 is SchemaName. If so, slightly better would be $0~CatalogName.$0~SchemaName
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r178909282
--- Diff: core/sql/exp/ExpError.cpp ---
@@ -135,6 +135,20 @@ ComDiagsArea *ExRaiseSqlError(CollHeap* heap, ComDiagsArea** diagsArea,
intParam1, intParam2, intParam3,
stringParam1, stringParam2, stringParam3);
}
+
+ComDiagsArea *ExRaiseSqlError(CollHeap* heap, ComDiagsArea** diagsArea,
--- End diff --
In many other places we use ComDiagsArea *&diags, so I'm wondering why we are using ** here.
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r178896649
--- Diff: core/sql/executor/ExExeUtilCli.cpp ---
@@ -2091,27 +2084,57 @@ Lng32 ExeCliInterface::deleteContext(char* contextHandle) // in buf contains con
Lng32 ExeCliInterface::retrieveSQLDiagnostics(ComDiagsArea *toDiags)
{
Lng32 retcode;
-
+ ex_assert(toDiags != NULL, "ComDiagsArea is null");
if (diagsArea_ != NULL)
{
diagsArea_->clear();
diagsArea_->deAllocate();
}
+ retcode = SQL_EXEC_MergeDiagnostics_Internal(*toDiags);
+ SQL_EXEC_ClearDiagnostics(NULL);
+ return retcode;
+}
- if (toDiags != NULL)
- {
- retcode = SQL_EXEC_MergeDiagnostics_Internal(*toDiags);
- SQL_EXEC_ClearDiagnostics(NULL);
- }
- else
+
+ComDiagsArea *ExeCliInterface::allocAndRetrieveSQLDiagnostics(ComDiagsArea *&toDiags)
+{
+ Lng32 retcode;
+
+ if (diagsArea_ != NULL)
{
- diagsArea_ = ComDiagsArea::allocate(heap_);
- retcode = SQL_EXEC_MergeDiagnostics_Internal(*diagsArea_);
+ diagsArea_->clear();
+ diagsArea_->deAllocate();
}
+ if (toDiags == NULL)
+ toDiags = ComDiagsArea::allocate(heap_);
- return retcode;
+ retcode = SQL_EXEC_MergeDiagnostics_Internal(*toDiags);
+ SQL_EXEC_ClearDiagnostics(NULL);
+
+ if (retcode == 0)
+ return toDiags;
+ else
+ return NULL;
}
+void ExeCliInterface::retrieveOrSaveSQLDiagnostics(Lng32 cliRetcode, ComDiagsArea *diagsArea)
--- End diff --
A comment would help here that we are returning the diagnostics if diagsArea is non-null and that we are saving them if NULL is passed in.
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r178947965
--- Diff: core/sql/exp/ExpError.cpp ---
@@ -135,6 +135,20 @@ ComDiagsArea *ExRaiseSqlError(CollHeap* heap, ComDiagsArea** diagsArea,
intParam1, intParam2, intParam3,
stringParam1, stringParam2, stringParam3);
}
+
+ComDiagsArea *ExRaiseSqlError(CollHeap* heap, ComDiagsArea** diagsArea,
--- End diff --
This function is modeled after other ExRaiseSqlError functions already in this file. I wasn't sure how ComDiagsArea would be passed to the callers of ExRaiseSqlError, meaning if it would exist or not. If the ComDiagsArea is passed to the caller as a parameter to itself and then it would be become recursive change to ensure that all the function in the chain passes reference pointers.
The existing ExRaiseSqlError excepts the enum ExeErrorCode needs to be updated to raise sql error correctly. However, with the embedded arkcmp concepts, even the non-executor error codes needs to be passed to this function. Casting to enum ExeErrorCode when the code doesn't exist in it causes unexpected value. Hence this new function was introduced to raise sql error. Earlier, the code assumed that diagsArea is always allocated and populated it.
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r178940492
--- Diff: core/sql/executor/ExExeUtilCommon.cpp ---
@@ -669,7 +669,9 @@ short ExExeUtilTcb::executeQuery(char * task,
char * stringParam1 = NULL;
Lng32 intParam1 = ComDiags_UnInitialized_Int;
- retcode = (short)cliInterface()->retrieveSQLDiagnostics(getDiagsArea());
+ setDiagsArea(cliInterface()->allocAndRetrieveSQLDiagnostics(getDiagsArea()));
+ if (getDiagsArea() != NULL)
+ retcode = 0;
--- End diff --
Thanks for the explanation.
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r178906139
--- Diff: core/sql/executor/ExExeUtilGet.cpp ---
@@ -1818,8 +1813,9 @@ short ExExeUtilGetMetadataInfoTcb::work()
{
if (!CmpCommon::context()->isAuthorizationEnabled())
{
- ComDiagsArea * diags = getDiagsArea();
- *diags << DgSqlCode(-CAT_AUTHORIZATION_NOT_ENABLED);
+ ComDiagsArea * diagsArea = getDiagsArea();
+ ExRaiseSqlError(getHeap(), &diagsArea, -CAT_AUTHORIZATION_NOT_ENABLED);
+ setDiagsArea(diagsArea);
--- End diff --
Having to do these three statements in so many places is not ideal. Would it make sense to make diagsArea_ a protected member, so we could pass it to ExRaiseSqlError here, in just a single statement:
```
ExRaiseSqlError(getHeap(), &diagsArea_, ...);
```
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r178949774
--- Diff: core/sql/executor/ExExeUtilGet.cpp ---
@@ -1818,8 +1813,9 @@ short ExExeUtilGetMetadataInfoTcb::work()
{
if (!CmpCommon::context()->isAuthorizationEnabled())
{
- ComDiagsArea * diags = getDiagsArea();
- *diags << DgSqlCode(-CAT_AUTHORIZATION_NOT_ENABLED);
+ ComDiagsArea * diagsArea = getDiagsArea();
+ ExRaiseSqlError(getHeap(), &diagsArea, -CAT_AUTHORIZATION_NOT_ENABLED);
+ setDiagsArea(diagsArea);
--- End diff --
I thought about it 3 statements when I made this change. However, there are places where the diagsArea comes up from the queue entries.
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r178944088
--- 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 --
Yes. I could do that for ExExeUtilTcb portion of the code.
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r178932278
--- Diff: core/sql/executor/ExExeUtilCommon.cpp ---
@@ -669,7 +669,9 @@ short ExExeUtilTcb::executeQuery(char * task,
char * stringParam1 = NULL;
Lng32 intParam1 = ComDiags_UnInitialized_Int;
- retcode = (short)cliInterface()->retrieveSQLDiagnostics(getDiagsArea());
+ setDiagsArea(cliInterface()->allocAndRetrieveSQLDiagnostics(getDiagsArea()));
+ if (getDiagsArea() != NULL)
+ retcode = 0;
--- End diff --
Yes. We can rely on that assumption. It is strange that the earlier code sets the sqlcode to the return code of SQL_EXEC_MergeDiagnosticsInternal to be the sqlcode. See line 700 below. When we are able to obtain the diagnostics area, the sqlcode is set the main error code
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r178944754
--- Diff: core/sql/executor/ExExeUtilCli.cpp ---
@@ -2091,27 +2084,57 @@ Lng32 ExeCliInterface::deleteContext(char* contextHandle) // in buf contains con
Lng32 ExeCliInterface::retrieveSQLDiagnostics(ComDiagsArea *toDiags)
{
Lng32 retcode;
-
+ ex_assert(toDiags != NULL, "ComDiagsArea is null");
if (diagsArea_ != NULL)
{
diagsArea_->clear();
diagsArea_->deAllocate();
}
+ retcode = SQL_EXEC_MergeDiagnostics_Internal(*toDiags);
+ SQL_EXEC_ClearDiagnostics(NULL);
+ return retcode;
+}
- if (toDiags != NULL)
- {
- retcode = SQL_EXEC_MergeDiagnostics_Internal(*toDiags);
- SQL_EXEC_ClearDiagnostics(NULL);
- }
- else
+
+ComDiagsArea *ExeCliInterface::allocAndRetrieveSQLDiagnostics(ComDiagsArea *&toDiags)
+{
+ Lng32 retcode;
+
+ if (diagsArea_ != NULL)
{
- diagsArea_ = ComDiagsArea::allocate(heap_);
- retcode = SQL_EXEC_MergeDiagnostics_Internal(*diagsArea_);
+ diagsArea_->clear();
+ diagsArea_->deAllocate();
}
+ if (toDiags == NULL)
+ toDiags = ComDiagsArea::allocate(heap_);
- return retcode;
+ retcode = SQL_EXEC_MergeDiagnostics_Internal(*toDiags);
+ SQL_EXEC_ClearDiagnostics(NULL);
+
+ if (retcode == 0)
+ return toDiags;
+ else
+ return NULL;
--- End diff --
I will fix it by de-allocating if it is allocated before returning NULL
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r178898607
--- Diff: core/sql/executor/ExExeUtilCommon.cpp ---
@@ -669,7 +669,9 @@ short ExExeUtilTcb::executeQuery(char * task,
char * stringParam1 = NULL;
Lng32 intParam1 = ComDiags_UnInitialized_Int;
- retcode = (short)cliInterface()->retrieveSQLDiagnostics(getDiagsArea());
+ setDiagsArea(cliInterface()->allocAndRetrieveSQLDiagnostics(getDiagsArea()));
+ if (getDiagsArea() != NULL)
+ retcode = 0;
--- End diff --
Should we rely on the assumption that retcode is non-zero when we reach here? Maybe better to set it explicitly to some defined value, something indicating that an error occurred but that we found no diagnostics information?
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by zellerh <gi...@git.apache.org>.
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.
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r179548743
--- Diff: core/sql/bin/SqlciErrors.txt ---
@@ -2,7 +2,7 @@
1000 42000 99999 BEGINNER INFRM LOGONLY A syntax error occurred.
1001 ZZZZZ 99999 ADVANCED CRTCL DIALOUT An internal error occurred in module $0~String0 on line $1~Int0. Details($2~String1).
1002 ZZZZZ 99999 BEGINNER MAJOR DBADMIN Catalog $0~CatalogName does not exist.
-1003 ZZZZZ 99999 BEGINNER MINOR DBADMIN Schema $0~SchemaName does not exist.
+1003 ZZZZZ 99999 BEGINNER MINOR DBADMIN Schema $0~String0.$1~String1 does not exist.
--- End diff --
There are two ways to raise an error.
1) Using ExRaiseSqlError method passing in sqlcode, string and numeric parameters
2) concatenating SQLError code and the parameters via ComDiagsArea << DgSqlCode << DgStringParam0 or DgCatalogName etc.
Option 1 doesn't support the concept of DgCatalogName or DgSchemaName .
When an error needs to be populated in both ways, we had to change it to use String0 and String1 etc..
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r179279239
--- Diff: core/sql/executor/ExExeUtilGet.cpp ---
@@ -1818,8 +1813,9 @@ short ExExeUtilGetMetadataInfoTcb::work()
{
if (!CmpCommon::context()->isAuthorizationEnabled())
{
- ComDiagsArea * diags = getDiagsArea();
- *diags << DgSqlCode(-CAT_AUTHORIZATION_NOT_ENABLED);
+ ComDiagsArea * diagsArea = getDiagsArea();
+ ExRaiseSqlError(getHeap(), &diagsArea, -CAT_AUTHORIZATION_NOT_ENABLED);
+ setDiagsArea(diagsArea);
--- End diff --
I misunderstood your comment. I thought that you wanted to make a function. Yes. I am changing it to pass in diagsArea_ directly, so that we can do this in a single statement. diagsArea_ already has protected mode access.
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r179533330
--- Diff: core/sql/sqlcomp/CmpSeabaseDDLschema.cpp ---
@@ -410,7 +410,8 @@ Int64 schemaUID = getObjectTypeandOwner(&cliInterface,
if (schemaUID < 0)
{
*CmpCommon::diags() << DgSqlCode(-CAT_SCHEMA_DOES_NOT_EXIST_ERROR)
- << DgSchemaName(catalogName + "." + schemaName);
+ << DgString0(catalogName)
--- End diff --
Consider using DgCatalogName and DgSchemaName instead of DgString0 and DgString1. If we build a manageability infrastructure that collects event data, it is good for the data to by typed by token.
---
[GitHub] trafodion pull request #1504: [TRAFODION-3009] Streamline error handling in ...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1504#discussion_r178970491
--- Diff: core/sql/exp/ExpError.cpp ---
@@ -135,6 +135,20 @@ ComDiagsArea *ExRaiseSqlError(CollHeap* heap, ComDiagsArea** diagsArea,
intParam1, intParam2, intParam3,
stringParam1, stringParam2, stringParam3);
}
+
+ComDiagsArea *ExRaiseSqlError(CollHeap* heap, ComDiagsArea** diagsArea,
--- End diff --
Thanks for the explanation.
---