|
Based on the comments so far, I'm guessing that the way to fix this is to extend HasVariantValueNodeVistor.visit(Visitable v) with special handling of ResultColumn objects. It doesn't seem reasonable to make all NumericConstantNodes be not constant, and ResultColumn appears to be where information about default value/auto-increment is kept.
ResultColumn has, what appears to be, four relevant predicates: isAutoincrement() isAutoincrementGenerated() isDefaultColumn() isGenerated() For the failing example, only isAutoincrementGenerated() returns true. HasVariantValueNodeVistor.visit(Visitable v) does not consider any of them. The logic in ResultColumn.getOrderableVariantType() looks incorrect. The attached patch makes it return Qualifier.VARIANT for all auto-increment generated columns. I haven't run any regression tests, but it makes the repro run without failure in my sandbox.
I agree, but I'm also worried about what happens when some of the other predicates are true. Will that ever happen, and could it cause problems? We should probably have some more test cases for default values that change between executions, but I don't know all the different ways in which this could happen.
Attaching an ij script (defaults.sql) which tests most of the legal default/autogen cases, (did not try to modify user between executions and did not test current time and current date). The error appears for all GENERATED AS IDENTITY columns when the default is specified explicitly, e.g. INSERT INTO T(I) VALUES(default) fails, but INSERT INTO T(X) VALUES(<something>) works correctly. INT, SMALLINT and BIGINT all have the same problem.
Knut's patch appears to fix all these cases. I just realized that the behavior I described in my previous comment matches perfectly with the problem pointed out by Knut. Presumably ResultColumn.expression is non-null exactly when there is no VALUES(default) clause. Which, in turn, explains why the isAutoincrementGenerated() predicate only gets evaluated when there is no explicit VALUES(default) clause.
The comment in ResultColumn.getOrderableVariantType() says "note that there is no expression associated with an autoincrement column in an insert statement." As noted by Dyre, this statement is false, as values(default) will cause a non-null expression also when the column is auto-generated. I therefore think d3343.diff is a correct fix (it should also update the incorrect comment). The tests (derbyall + suites.All) ran cleanly.
As to the predicates that are false, I think they could be explained. The default node is replaced with a placeholder node for the auto-generated value during the bind phase, so it is correct that isDefaultColumn() returns false. isAutoincrement() seems to be used for columns that reference an auto-increment column in a table in a select statement, whereas values(default) doesn't actually reference a column in a table. isGenerated() seems to be used for a different kind of generated columns than isAutoincrementGenerated() (aggregated columns and internal helpers like row location columns). Given that information, I think I'll merge d3343.diff with a JUnit version of (parts of) the ij-script I uploaded, so that we get a regression test for the fix included in the patch.
Attaching new patch (v2) which fixes the incorrect comment, and adds regression tests. All tests (including the new regression tests pass).
Looks good to me. Two nits:
1) There's a mismatch between the parameter name and the @param tag in testGeneratedIdentity() (generateType vs generatedType) 2) I think it's best not to have cleanup code inside the test cases, like the try/finally block in testGeneratedIdentity(), since it tends to obscure failures. Since the test is already wrapped in a CleanDatabaseTestSetup, perhaps you could just make sure each invocation of the method uses a unique table name and rely on the decorator to do the cleanup. Attaching another patch (v3) which fixes the javadoc typo and removes the try-finally cleanup. It turns out that it was redundant, since setUp() turns autocommit off, and tearDown() issues a rollback().
I do, however, think that the assertion that using a try-finally block will obscure failures is not true in general. To test it I did the following: - reverted the changes to ResultColumn - set autocommit to true - introduced a syntax error in the drop statement in the finally block The drop-error was masked as expected but the call stack from JUnit was correct and showing the correct error message (duplicate key). The next testcase failed because table T already existed, as expected. It is entirely possible that using a try-finally block like this is "not the JUnit way". I would not know. I have not run any other tests with this (v3) patch. Thanks Dyre, +1 to commit.
> the assertion that using a try-finally block will obscure failures is not true in general. (...) > The drop-error was masked as expected but the call stack from JUnit was correct and showing the correct error message (duplicate key). > The next testcase failed because table T already existed, as expected. A syntax error in one test case being reported as "table T already exists" in another test case sounds obscure to me... ;) But isn't that exactly what would happen with your solution if someone inadvertently tried to use a table name already used by another test case...?
Yes, but in that case the problem is that the table exists, not that drop table failed, so "table T already exists" would actually be an accurate message. :)
I agree, of course, that it is certainly possible to use try/finally without obscuring the error reporting, and that it is very unlikely that the try/finally block you wrote would ever cause any problem. However, I do think that the test case became easier to read now that the cleanup code was moved out. Committed revision 615203.
I managed to commit this with a log message which reference a different jira number (thanks to Knut for noticing). I have updated the log comment (in case anyone wonders why the output from svn log has changed).
Adding fix-version 10.3.2.2 since the other
Merged to 10.3 in revision 615458.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Resolved
Major
Subsequent calls to PreparedStatement cause SQLIntegrityConstraintViolationException on column that is "Generated always"
DERBY-827