Details

Description

In contrast to generated column, which are evaluated when the next row from the result set to be inserted, currently default values and identity columns are generated "early", that is as part of avaluating the subquery (SELECT or VALUES as the case may be).
This does not currently cause a user visible bug in Derby, but it lies behind DERBY-3 and the effect Bryan observed in DERBY-4.
Additionally, "early" computation has given rise to much special handling and ensuing bugs, cf. DERBY-1644, DERBY-4413, DERBY-4419, DERBY-4425 and others.

It looks like the fix for DERBY-4419/DERBY-4425 can also be backed out now, because a placeholder for an autoincrement column or a generated column cannot appear in a result column list that far down in the node tree anymore. I'm running the full regression test suite now.

Knut Anders Hatlen
added a comment - 04/Dec/09 14:09 It looks like the fix for DERBY-4419 / DERBY-4425 can also be backed out now, because a placeholder for an autoincrement column or a generated column cannot appear in a result column list that far down in the node tree anymore. I'm running the full regression test suite now.

Thanks Dag. I've committed the patch to trunk with revision 885421, since it looks like DERBY-4426 is more or less ready for commit too.

I'll leave this issue open until I've investigated if we can back out the fixes for some of the other issues linked to this one now. I think at least there were some asserts that were loosened to accept untyped nulls (placeholders for the identity values) at odd places in the code, and these could now be made stricter again to potentially catch other errors.

Knut Anders Hatlen
added a comment - 30/Nov/09 13:44 Thanks Dag. I've committed the patch to trunk with revision 885421, since it looks like DERBY-4426 is more or less ready for commit too.
I'll leave this issue open until I've investigated if we can back out the fixes for some of the other issues linked to this one now. I think at least there were some asserts that were loosened to accept untyped nulls (placeholders for the identity values) at odd places in the code, and these could now be made stricter again to potentially catch other errors.

Knut Anders Hatlen
added a comment - 27/Nov/09 15:42 The patch is ready for review, so I'm setting "Patch Available". We should probably wait for DERBY-4426 to be fixed before the patch is committed to prevent exposing the NPEs mentioned earlier.

The comment in InsertNode.enhanceAndCheckForAutoincrement() only explained why it was OK to skip the call to forbidOverrides() on the top-level RCL for a multi-row table constructor. Now it also explains why it should be skipped, to make it clear that the check is actually needed and not some kind of optimization.

Knut Anders Hatlen
added a comment - 27/Nov/09 15:33 Here's a new revision of the patch (1b) with one small change:
The comment in InsertNode.enhanceAndCheckForAutoincrement() only explained why it was OK to skip the call to forbidOverrides() on the top-level RCL for a multi-row table constructor. Now it also explains why it should be skipped, to make it clear that the check is actually needed and not some kind of optimization.

Here's an updated patch (d4442-1a.diff). I've added comments to the new methods, updated the existing comments to better match the current state of the code, updated the DERBY-3 test case to expect the new behaviour, and also added a test case for DERBY-4433.

Knut Anders Hatlen
added a comment - 20/Nov/09 21:21 Here's an updated patch (d4442-1a.diff). I've added comments to the new methods, updated the existing comments to better match the current state of the code, updated the DERBY-3 test case to expect the new behaviour, and also added a test case for DERBY-4433 .

Knut Anders Hatlen
added a comment - 20/Nov/09 16:10 The patch does break some statements that used to work, for instance this:
ij> create table t2(x int generated by default as identity);
0 rows inserted/updated/deleted
ij> insert into t2 values 1 union values default;
ERROR XJ001: Java exception: ': java.lang.NullPointerException'.
However, I don't think this statement should have been accepted in the first place. VALUES DEFAULT is not accepted in a subquery unless it's part of a UNION, it seems:
ij> insert into t2 select * from (values default) v;
ERROR 42Y85: The DEFAULT keyword is only allowed in a VALUES clause when the VALUES clause appears within an INSERT statement.

Thanks, Dag. That's good news. I ran derbyall and suites.All with always_prn.diff, and only one test failed: DistinctTest.testDistinctInsertWithGeneratedColumn(). That test case is intended to fail if DERBY-3 is fixed, according to its comments, so that should be fine:

Knut Anders Hatlen
added a comment - 20/Nov/09 15:37 Thanks, Dag. That's good news. I ran derbyall and suites.All with always_prn.diff, and only one test failed: DistinctTest.testDistinctInsertWithGeneratedColumn(). That test case is intended to fail if DERBY-3 is fixed, according to its comments, so that should be fine:
See DERBY-3 . If that bug is fixed, the first query after the comment
below will fail.
I'll try to clean up the patch and get it into a committable shape.

Thanks for trying the patch with the DERBY-4 patch. Here is a slightly
modified InsertNode.java which (with your patch + a modified
contribution from DERBY-4) which seems to work.

As you can see, I moved the ORDER BY pulling and binding to happen
before the enhancement (not a problem any longer since we can't sort a
VALUES in the insert context), and dropped the use of the column map.

Dag H. Wanvik
added a comment - 20/Nov/09 14:23 Thanks for trying the patch with the DERBY-4 patch. Here is a slightly
modified InsertNode.java which (with your patch + a modified
contribution from DERBY-4 ) which seems to work.
As you can see, I moved the ORDER BY pulling and binding to happen
before the enhancement (not a problem any longer since we can't sort a
VALUES in the insert context), and dropped the use of the column map.
ij> select * from t4;
I |J
-----------------------
1 |200
2 |100
3 |300
ij> insert into t3 select j from t4 order by j;
ij> select * from t3;
ID |I
-----------------------
34 |100
35 |200
36 |300

I took the prn3 patch attached to DERBY-4433, which ensures that generated/identity columns are added in a ProjectRestrictNode on top of the source result set if the source is a SetOperatorNode, and moved the PRN generation from SetOperatorNode to ResultSetNode so that it applies to all kinds of result sets (except table constructors, which need special handling). See the attached always_prn patch.

With that patch, the DERBY-3 repro does no longer produce gaps in the identity values, so the approach looks promising. I also tried it in combination with the DERBY-4 patch (derby-4_dhw), but then it unfortunately produced an assert error:

Knut Anders Hatlen
added a comment - 20/Nov/09 11:22 I took the prn3 patch attached to DERBY-4433 , which ensures that generated/identity columns are added in a ProjectRestrictNode on top of the source result set if the source is a SetOperatorNode, and moved the PRN generation from SetOperatorNode to ResultSetNode so that it applies to all kinds of result sets (except table constructors, which need special handling). See the attached always_prn patch.
With that patch, the DERBY-3 repro does no longer produce gaps in the identity values, so the approach looks promising. I also tried it in combination with the DERBY-4 patch (derby-4_dhw), but then it unfortunately produced an assert error:
ij> create table t3(id int generated always as identity, i int);
0 rows inserted/updated/deleted
ij> insert into t3 select * from t1 order by x;
ERROR XJ001: Java exception: 'ASSERT FAILED bindExpressions() is not expected to be called for class org.apache.derby.impl.sql.compile.ProjectRestrictNode: org.apache.derby.shared.common.sanity.AssertFailure'.