|
I was running the above queries for kicks to see if I could reproduce the problem--but they all succeeded. I then realized that my test table "t1" only had *one* column--the "generated by default" column c2. When I recreated the table so that it had two columns (c1 and c2 as posted in the repro) the NPE occurs.
So the bug appears to require that there be at least one non-identity column in the table, as well. Maybe that's useful, maybe not... Thanks Army! I think that your observation is crucial, and here's why:
The problem arises due to this code in InsertNode.bind(): if (! inOrder || resultSet.resultColumns.size() < numTableColumns) { // one thing we do know is that all of the resultsets underneath // us have their resultColumn names filled in with the names of // the target table columns. That makes generating the mapping // "easier" -- we simply generate the names of the target table columns // that are included. For the missing columns, we generate default // value expressions. resultSet = resultSet.enhanceRCLForInsert(numTableColumns, colMap, dataDictionary, targetTableDescriptor, targetVTI); } if (resultSet instanceof UnionNode) { // If we are inserting a number of rows in VALUES clause, we need to // examine each row for 'autoincrement'. resultColumnList.checkAutoincrementUnion(resultSet); } else resultColumnList.checkAutoincrement(resultSet.getResultColumns()); When the VALUES clause encounters multiple rows, it generates a UNION node tree to combine the rows to be inserted. The code above notices the top-level UNION node and calls the special checkAutoincrementUnion() method which knows how to recursively traverse the Union tree and call checkAutoIncrement() on the underlying RowResultSetNode instances at the leaf level of the tree. HOWEVER, when the number of columns in the rows in the VALUES clause is a subset of the number of columns in the table we're inserting into, the top node of the tree is not a UnionNode, but is rather a ProjectRestrictNode. This means that we skip past the UnionNode test and just call checkAutoincrement(), which processes the PRN but doesn't go down to the RowResultSetNode(s) at the leaf level. This leaves the ResultColumn instance at the leaf level with a NULL column descriptor, which causes the NPE during the code generation phase. And, there is a second, related problem. The enhanceRCLForInsert() call is also only made at the top level of the tree. However, this call is a necessary pre-condition for calling checkAutoincrement() because enhanceRCLForInsert() ensures that the proper ResultColumnList values are in place prior to the checkAutoincrement() reconciliation of the column lists. I believe that the way to solve this is to merge the above code from InsertNode.bind together with the current recursive processing in ResultColumnList.checkAutoincrementUnion() to produce a new recursive routine, which I have called enhanceAndCheckForAutoincrement(), which will recursively traverse the ResultSet tree, calling *both* enhanceRCLForInsert() and checkAutoincrement() on the various nodes in the tree. That way, even if the RowResultSetNode instances are buried within UnionNode trees, they will still be correctly set up for autoincrement processing during bind processing. I'll attach a complete patch to this Jira issue once I finish writing a bunch of tests and tidying up the code changes. Attached is d1644_recursivelyCheck_v1.diff, a proposed patch to fix this issue.
This patch moves the method checkAutoincrementUnion() from ResultColumnList into InsertNode, renames the method to enhanceAndCheckForAutoincrement(), and updates the recursive processing so that it: a) can handle PRN nodes in the ResultSet tree b) knows to call enhanceRCLForInsert on ResultSetNodes c) can recurse on all sorts of ResultSet trees, not just those headed by a UnionNode. The patch also contains some new tests. The patch passes derbyall and suites.All, and I'd love to hear your feedback on it. I think there is room for a follow-on patch in this area, to ResultSetNode.enhanceRCLForInsert.
The Javadoc comments for this method suggest that this is the place where PRN nodes can get inserted into the ResultSet node tree, but in fact this method does not do this. I suspect that perhaps it once did, and it could be that when that processing was moved elsewhere, that is when I think it would be useful to have a follow-on patch to ResultSetNode, which would have no functional changes, but would improve the clarity of the code, to do the following: 1) Change the return type of enhanceRCLForInsert from ResultSetNode to void 2) Remove the "return this" statement 3) Remove the unused variable numResultSetColumns 4) Fix the Javadoc for this method to remove the suggest that it may insert a PRN into the ResultSet tree. I shall attach such a follow-on patch for review. RSNCommentFixup_v1.diff, which must be applied after the recursivelyCheck patch,
is an optional second part to the patch which, I believe, improves the clarity of ResultSetNode.enhanceRCLForInsert(). I would appreciate any feedback about whether this patch is worth including as part of this change. Hmmm... I think that second patch is mis-considered. I see now that
SetOperatorNode.enhanceRCLForInsert overrides ResultSetNode.enhanceRCLForInsert, and the SetOperatorNode version *does* return a new result set node. I think this means that I still don't understand this problem well enough. Feedback on the patch(es) is most welcome, but they are definitely not ready for commit. I think the method SetOperatorNode.enhanceRCLForInsert can
be wholly deleted, leaving only the implementation in ResultSetNode. I shall pursue this possibility. After doing some more studying and testing, I believe that it is in fact
safe to remove SetOperatorNode.enhanceRCLForInsert() entirely, and to simply ResultSetNode.enhanceRCLForInsert() as described in my previous comment. There seems to be no code path which can all SetOperatorNode.enhanceRCLForInsert(). Attached is RSN_EnhanceRCL_Simplify_v2.diff, which is as I said an optional part of this fix, but I think it is nice to delete code if it is clearly unnecessary because that simplifies the overall implementation. Please let me know your feedback. Hi Bryan,
I applied the d1644_recursivelyCheck_v1.diff patch to my local codeline and it applied cleanly. However, when I tried to run lang/autoincrement.sql the test failed due to an ArrayIndexOutOfBounds exception. I was able to reproduce the error on a clean database by executing the following (pulled from that test): ij> create table ai_test (x int generated always as identity (start with 2, increment by 2), y int); 0 rows inserted/updated/deleted ij> insert into ai_test (y) values (1),(2); ERROR XJ001: Java exception: '1 >= 1: java.lang.ArrayIndexOutOfBoundsException'. java.lang.ArrayIndexOutOfBoundsException: 1 >= 1 at java.util.Vector.elementAt(Vector.java(Compiled Code)) at org.apache.derby.impl.sql.compile.QueryTreeNodeVector.elementAt(QueryTreeNodeVector.java:52) at org.apache.derby.impl.sql.compile.ResultColumnList.checkStorableExpressions(ResultColumnList.java:890) at org.apache.derby.impl.sql.compile.InsertNode.bind(InsertNode.java:419) at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:345) at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:119) at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConnectionContext.java:742) at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:568) at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:517) at org.apache.derby.impl.tools.ij.ij.executeImmediate(ij.java:321) at org.apache.derby.impl.tools.ij.utilMain.doCatch(utilMain.java:517) at org.apache.derby.impl.tools.ij.utilMain.runScriptGuts(utilMain.java:370) at org.apache.derby.impl.tools.ij.utilMain.go(utilMain.java:268) at org.apache.derby.impl.tools.ij.Main.go(Main.java:204) at org.apache.derby.impl.tools.ij.Main.mainCore(Main.java:170) at org.apache.derby.impl.tools.ij.Main14.main(Main14.java:56) at org.apache.derby.tools.ij.main(ij.java:71) To see if it made any difference I also applied RSN_EnhanceRCL_Simplify_v2.diff to my codeline. When I did that things started working correctly again. This surprised me since I was under the impression that this second patch was optional--i.e. nice but not required. Am I overlooking something here, or is it in fact true that both patches must be applied in order for your fix to be complete? I didn't look into it at all but if you could give a quick explanation of why the second patch is required, that'd be great. In any event, having applied both patches together I was able to run lang/autoincrement.sql without problems. I also verified that without the changes the new tests fail with an NPE, as expected. My one minor suggestion is to perhaps add two more test cases to your current scenario: insert into D1644 (c2) values default, default, default, default; insert into D1644 (c2, c1) values (default, 128), (default, 129), (default, 131); The above two INSERTs work as expected when both of your patches have been applied, so this is not a requirement. But it might be nice to have the extra test cases, just for safety... Thanks for the review Army! I had both patches together in my sandbox and thought
I had separated them cleanly, but it seems that they got hooked together. I shall go study this some more. I'm not quite sure what I was thinking when I thought that the two code
changes could be separated, but they cannot. They are linked and really it needs to be a single patch proposal. Attached is d1644_combined_v3.diff, a patch proposal which combines the two previous code change patches. The new patch also adds the new tests suggested by Army. There are several ideas involved in this patch: - When an INSERT statement will insert multiple rows from the VALUES clause, the compiler will compile the various values into a tree of UnionNodes with RowResultSetNodes at the leaves of the three - The columns specified in the INSERT statement may be a subset of the rows in the table. The "extra" columns need to be constructed by the INSERT statement, either by generating NULL values for those columns which are nullable, or by compiling a default values for those columns which have DEFAULT values, or by generating a value for an IDENTITY column which is GENERATED. The work of constructing these extra column values is done by genNewRCForInsert. - For columns which are GENERATED ALWAYS, we must make sure that the INSERT statement doesn't allow the user to insert their own value for the generated column. - The columns which are specified in the INSERT column spec may not match the order in which the columns arise in the table. Therefore, the column values may need to be re-ordered by the INSERT statement so that they occur in the proper order. - In the case when the ResultSet which provides the values for the INSERT statement is not just a single node, but is rather a tree of UnionNodes, the above processing needs to happen throughout the tree, not just at the root node. The combined patch proposal accomplishes all of this. There is one aspect to this patch that I can't fully explain yet. If somebody knows the reason behind it, that would be very nice; I am hoping to continue studying this in the near future. This question involves the ProjectRestrictNode at the root of the ResultSetNode tree. With the current code, when the VALUES clause results in multiple rows, and is compiled into a UnionNode tree with RowResultSetNodes at the leaves, InsertNode.bind calls SetOperatorNode. enhanceRCLForInsert(), which constructs a brand-new ProjectRestrictNode to be the new root of the UnionNode tree. With my patch proposal, the PRN is no longer built; the ResultSetNode tree is left with a UnionNode at the root. From what I can tell, this works fine: the PRN is no longer needed once we are sure to generate and re-order the ResultSet columns at *all* nodes in the ResultSetNode tree. The new rows can be inserted directly from the UnionNodes. I think this patch is correct, but it is possible that there is some other aspect to having the PRN at the root of the tree which I haven't discovered yet, which will cause a problem if we don't generate that PRN. If anybody knows what such a problem might be, please let me know. Thank you for the updated patch, Bryan. I was able to apply it cleanly to trunk and I ran the lang/autoincrement.sql test without any problems. Code comments look good and the changes agree with the explanation of the problem/solution that you provided for this issue.
As for your question about the PRN over the UnioNode, I agree that the PRN seems unnecessary based on my understanding of the changes. As you mentioned in a previous comment, prior to your changes the enhanceRCLForInsert() call was only made at the top of the tree. In the case of a UnionNode the code did the "enhancing" by generating a PRN over the UnionNode, where the PRN itself had the "enhanced" result columns. The rest of the tree was then left untouched. But since, as you have said, your changes make it so that the entire tree beneath the UnionNode (including the UnionNode itself) is "enhanced", the additional PRN is no longer required. So basically, I agree with your analysis and I think your solution looks good. Assuming, of course, that derbyall ran cleanly? ;) I should say, though, that after re-reading the code comments for enhanceRCLForInsert() method several times, these lines still have me scratching my head: * Those RSNs whose generate() method does not handle projects will * insert a PRN, with a new RCL which matches the target RCL, above * the current RSN. I see that you removed these lines in your patch so that the comment matches the current state of the code, which is good. But I'm having problems figuring out what the lines were saying to begin with. In particular, what does it mean to say "those RSNs whose generate() method does not handle projects"? In any event, your changes make sense and the basic tests I've run all pass, so I too tend to think the patch is correct. Assuming derbyall runs cleanly, I vote +1 to commit. I wasn't able to apply the latest patch, d1644_combined_v3.diff cleanly. It has a problem patching ResultColumnList.java, so I have to resolve this manually. The patch fixes the stated problem of this jira and without it, it fails. I have reviewed the code changes and they all look reasonable to me. (Great comments and thanks for cleaning up the code to make it clearer for reviewers/contributers). I also ran derbyall + junit suite and they all pass. +1 to commit.
Thank you very much Army and Yip for the reviews. I'll bring the patch up to date and look into
proceeding with some final testing and commit. Committed d1644_combined_v3.diff to the trunk as revision 487414.
I haven't marked this issue resolved yet because I suspect we may want to investigate merging these changes back to the 10.2 branch. Yip, what do you think? Should we merge these changes back to 10.2? After thinking about it some more I decided to mark this issue as resolved,
since the change is in the trunk. If we later decide that we want to port this change to one or more of the older branches, we can re-open the issue then. Revision 487656 is a follow-on commit with a 1-line fix to a javadoc typo
parameter targetTD should have been targetTableDescriptor. This issue is resolved and has not been updated in the last 12 months (since 24/Jan/07).
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Minor
NPE when inserting values to a table that has a column declared as generated by default as identity
Note that the quite-similar-but-not-identical statements below all work fine:
insert into t1 (c2) values (10);
insert into t1 (c2) values default;
insert into t1 (c2) values (default);
insert into t1 (c2) values 10, 20, 30;
However, this statement gets the same NPE as the one in the description:
insert into t1 (c2) values 40, 50, default;
So it appears to be that the bug involves:
- inserting multiple rows with a single INSERT statement using multiple values clauses, and
- some of the rows have numeric constant values, while others fire the identity field generator