Derby
  1. Derby
  2. DERBY-4442

Evaluation of default value and identity in an INSERT result set evaluated too early.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.5.3.0
    • Fix Version/s: 10.5.3.1, 10.6.1.0
    • Component/s: SQL
    • Labels:
      None
    • Bug behavior facts:
      Deviation from standard

      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.

      DERBY-4397 requires this fix for correct behaviour with INSERT.

      See also
      https://issues.apache.org/jira/browse/DERBY-4413?focusedCommentId=12769532&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12769532

      1. always_prn.diff
        11 kB
        Knut Anders Hatlen
      2. insert.diff
        7 kB
        Dag H. Wanvik
      3. d4442-1a.stat
        0.5 kB
        Knut Anders Hatlen
      4. d4442-1a.diff
        17 kB
        Knut Anders Hatlen
      5. d4442-1b.diff
        17 kB
        Knut Anders Hatlen
      6. backout-derby-4425.diff
        0.6 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Hide
          Mike Matrigali added a comment -

          backported all changes for this issue from trunk to 10.5 branch, resolving as fixed in 10.5, and resetting original owner.

          Show
          Mike Matrigali added a comment - backported all changes for this issue from trunk to 10.5 branch, resolving as fixed in 10.5, and resetting original owner.
          Hide
          Mike Matrigali added a comment -

          backporting svn #888311 from trunk to 10.5 branch.

          m105_jdk16:111>svn commit
          Sending .
          Sending java/engine/org/apache/derby/impl/sql/compile/ResultColumnList.java
          Transmitting file data .
          Committed revision 960910.

          Show
          Mike Matrigali added a comment - backporting svn #888311 from trunk to 10.5 branch. m105_jdk16:111>svn commit Sending . Sending java/engine/org/apache/derby/impl/sql/compile/ResultColumnList.java Transmitting file data . Committed revision 960910.
          Hide
          Mike Matrigali added a comment -

          backporting change #885421 from trunk to 10.5 branch.

          m105_jdk16:76>svn commit
          Sending .
          Sending java/engine/org/apache/derby/impl/sql/compile/InsertNode.java
          Sending java/engine/org/apache/derby/impl/sql/compile/ResultSetNode.java
          Sending java/engine/org/apache/derby/impl/sql/compile/RowResultSetNode.java
          Sending java/engine/org/apache/derby/impl/sql/compile/UnionNode.java
          Sending java/testing/org/apache/derbyTesting/functionTests/master/intersect.out
          Sending java/testing/org/apache/derbyTesting/functionTests/tests/lang/DistinctTest.java
          Sending java/testing/org/apache/derbyTesting/functionTests/tests/lang/intersect.sql
          Transmitting file data .......
          Committed revision 960731.

          Show
          Mike Matrigali added a comment - backporting change #885421 from trunk to 10.5 branch. m105_jdk16:76>svn commit Sending . Sending java/engine/org/apache/derby/impl/sql/compile/InsertNode.java Sending java/engine/org/apache/derby/impl/sql/compile/ResultSetNode.java Sending java/engine/org/apache/derby/impl/sql/compile/RowResultSetNode.java Sending java/engine/org/apache/derby/impl/sql/compile/UnionNode.java Sending java/testing/org/apache/derbyTesting/functionTests/master/intersect.out Sending java/testing/org/apache/derbyTesting/functionTests/tests/lang/DistinctTest.java Sending java/testing/org/apache/derbyTesting/functionTests/tests/lang/intersect.sql Transmitting file data ....... Committed revision 960731.
          Hide
          Mike Matrigali added a comment -

          working on backporting this issue to 10.5. DERBY-4419, DERBY-4413, DERBY-4425, and DERBY-4442 all seem related. I am going to apply and checkin the backported changes to these issues in order. I think the changes necessary are:
          DERBY-4413 #829410
          DERBY-4419 #831304
          DERBY-4425 #831319
          DERBY-4442 #885421
          DERBY-4413 #885659
          DERBY-4442 #888311

          Show
          Mike Matrigali added a comment - working on backporting this issue to 10.5. DERBY-4419 , DERBY-4413 , DERBY-4425 , and DERBY-4442 all seem related. I am going to apply and checkin the backported changes to these issues in order. I think the changes necessary are: DERBY-4413 #829410 DERBY-4419 #831304 DERBY-4425 #831319 DERBY-4442 #885421 DERBY-4413 #885659 DERBY-4442 #888311
          Hide
          Lily Wei added a comment -

          Reopen to 10.5 back port

          Show
          Lily Wei added a comment - Reopen to 10.5 back port
          Hide
          Knut Anders Hatlen added a comment -

          Committed revision 888311. I'm not aware of any more work required for this issue. Marking as resolved.

          Show
          Knut Anders Hatlen added a comment - Committed revision 888311. I'm not aware of any more work required for this issue. Marking as resolved.
          Hide
          Knut Anders Hatlen added a comment -

          The attached patch backs out the fix for DERBY-4425 (except the tests), since it's not needed anymore. All the regression tests ran cleanly.

          Show
          Knut Anders Hatlen added a comment - The attached patch backs out the fix for DERBY-4425 (except the tests), since it's not needed anymore. All the regression tests ran cleanly.
          Hide
          Knut Anders Hatlen added a comment -

          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.

          Show
          Knut Anders Hatlen added a comment - 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.
          Hide
          Dag H. Wanvik added a comment -

          Backed out the assert relaxation introduced for DERBY-4413, no longer needed.

          Show
          Dag H. Wanvik added a comment - Backed out the assert relaxation introduced for DERBY-4413 , no longer needed.
          Hide
          Knut Anders Hatlen added a comment -

          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.

          Show
          Knut Anders Hatlen added a comment - 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.
          Hide
          Dag H. Wanvik added a comment -

          Patch looks good to me; +1 to commit (before or after DERBY-4426, your call).

          Show
          Dag H. Wanvik added a comment - Patch looks good to me; +1 to commit (before or after DERBY-4426 , your call).
          Hide
          Knut Anders Hatlen added a comment -

          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.

          Show
          Knut Anders Hatlen added a comment - 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.
          Hide
          Knut Anders Hatlen added a comment -

          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.

          Show
          Knut Anders Hatlen added a comment - 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.
          Hide
          Knut Anders Hatlen added a comment -

          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.

          Show
          Knut Anders Hatlen added a comment - 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 .
          Hide
          Knut Anders Hatlen added a comment -

          Dag's comments in DERBY-4426 suggest that the statement that causes NPE is non-standard.

          Show
          Knut Anders Hatlen added a comment - Dag's comments in DERBY-4426 suggest that the statement that causes NPE is non-standard.
          Hide
          Knut Anders Hatlen added a comment -

          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.

          Show
          Knut Anders Hatlen added a comment - 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.
          Hide
          Knut Anders Hatlen added a comment -

          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.

          Show
          Knut Anders Hatlen added a comment - 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.
          Hide
          Dag H. Wanvik added a comment -

          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

          Show
          Dag H. Wanvik added a comment - 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
          Hide
          Knut Anders Hatlen added a comment -

          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'.

          Show
          Knut Anders Hatlen added a comment - 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'.

            People

            • Assignee:
              Knut Anders Hatlen
              Reporter:
              Dag H. Wanvik
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development