GitHub user DaveBirdsall opened a pull request:
https://github.com/apache/incubator-trafodion/pull/1114
[TRAFODION-2635] Fix two bugs re: HBASE CELL access on Traf objects
This set of changes fixes two bugs.
1. In ValueIdList::computeEncodedKey (optimizer/ValueDesc.cpp), if we used
HBASE.\â_CELL_\â access for a salted Trafodion table, and we had no
begin/end key (e.g. select count(*) from
hbase.\â_CELL_\â.\âTRAFODION.SCH.XYZ\â), the code would dereference a
null pointer.
2. In NATableDB::get, if we were creating an NATable object on the NATable
cache for an HBASE.\â_CELL_\â access to a Trafodion table, the
HBase-related descriptors were allocated on the statement heap instead of the
context heap, resulting in a dangling pointer (with unpredictable failure
modes) when the object was accessed that way in a second statement.
The change to optimizer/ValueDesc.cpp fixes the first bug.
The remaining changes fix the second bug.
Why so many changes for the second bug?
Partly, it is because routines in generator/Generator.cpp that wrote these
descriptors into compiled plans were reused to obtain these descriptors for
compile time logic. Unfortunately, the memory models are different: For
compiled plans we use ComSpace for heap allocation (as we need the allocations
to be contiguous in memory). For compiler access, we use an NAHeap. Before this
set of changes, this was hidden in the GENHEAP macro in sqlcat/TrafDDLdesc.h;
the code would use a ComSpace object if the space parameter to several methods
was non-null, and use the statement heap if the space parameter was null.
The need to (sometimes) use the context heap in NATableDB::get meant that
this GENHEAP scheme could no longer work. Since ComSpace and NAHeap inherit
from NAMemory, one possibility was to change all relevant descriptor methods to
use an NAMemory * parameter instead of ComSpace *. This set of changes attempts
this possibility. There is a nuance: The NAMemory class hierarchy does not use
virtual methods. So, when there is a need for polymorphism, the calling
routines must explicitly cast the NAMemory pointer to the proper class.
Fortunately, there was only one place where this was necessary. Iâm not
completely satisfied with this solution as it seems a dangerous coding
practice. No simple alternative seemed to be available however.
After making this change, I encountered numerous test failures due to
uninitialized variables in the various descriptors. Perhaps objects on the
Statement heap are guaranteed (or vastly more likely) to be zeroed out while
those on the Context heap are not. As a result, I added initializers for many
of the descriptor members in sqlcat/TrafDDLdesc.h. I am not confident that I
got all of the ones needed (I did initialize all pointer members so those are
safe).
Please do not merge this until a full regression suite run has passed,
which Iâm doing on a workstation.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/DaveBirdsall/incubator-trafodion Trafodion2635
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-trafodion/pull/1114.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 #1114
----
commit 8d9c1d173d77adfa0fc274dac6b9ce89dc1aae51
Author: Dave Birdsall <[email protected]>
Date: 2017-06-07T22:53:38Z
[TRAFODION-2635] Fix two bugs concerning HBASE CELL access on Traf objects
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
