Derby
  1. Derby
  2. DERBY-5121

Data corruption when executing an UPDATE trigger

    Details

    • Urgency:
      Blocker
    • Issue & fix info:
      Release Note Needed, Repro attached
    • Bug behavior facts:
      Data corruption, Regression, Seen in production

      Description

      When executing an UPDATE trigger, the following error is raised. I will attach a test case:

      ERROR XCL12: An attempt was made to put a data value of type 'org.apache.derby.impl.jdbc.EmbedClob' into a data value of type 'INTEGER'.

      1. 5121_create.sql
        1 kB
        Rick Hillegas
      2. 5121_upgrade.sql
        0.1 kB
        Rick Hillegas
      3. DERBY_5121_backoutDerby1492_changes_patch3_trunk_diff.txt
        12 kB
        Mamta A. Satoor
      4. DERBY_5121_NotReadForCommit_patch3_trunk_diff.txt
        1 kB
        Mamta A. Satoor
      5. DERBY_5121_NotReadForCommit_patch4_trunk_diff.txt
        6 kB
        Mamta A. Satoor
      6. DERBY_5121_patch2_trunk_diff.txt
        4 kB
        Mamta A. Satoor
      7. derby5121_patch1_diff.txt
        4 kB
        Mamta A. Satoor
      8. derby-5121-01-aa-runTimeRemapping.diff
        13 kB
        Rick Hillegas
      9. DummyProc.java
        0.2 kB
        Rick Hillegas
      10. Proc_5121.java
        0.2 kB
        Rick Hillegas
      11. releaseNote.html
        4 kB
        Rick Hillegas
      12. releaseNote.html
        4 kB
        Mamta A. Satoor
      13. Test_5121_Upgrade.java
        20 kB
        Rick Hillegas
      14. Test_5121.java
        17 kB
        Rick Hillegas
      15. triggerBug.sql
        0.7 kB
        Rick Hillegas
      16. triggerBug.sql
        0.8 kB
        Rick Hillegas
      17. triggeredBug2.sql
        0.8 kB
        Rick Hillegas
      18. triggeredCorruption.sql
        0.7 kB
        Rick Hillegas

        Issue Links

          Activity

          Hide
          Rick Hillegas added a comment -

          Attaching DummyProc.java and triggerBug.sql, a repro for this issue. Compile DummyProc then run triggerBug via ij.

          Show
          Rick Hillegas added a comment - Attaching DummyProc.java and triggerBug.sql, a repro for this issue. Compile DummyProc then run triggerBug via ij.
          Hide
          Rick Hillegas added a comment -

          Changing the description of this bug since experiments show that it is more general. The triggered action seems to expect a row which just contains the columns necessary to execute the action. However, if the UPDATE touches columns which aren't mentioned by the trigger, those columns turn up in the row passed to the action. These extra columns can be in the middle of the action row. This throws off the column number of the compiled action and springs this bug.

          Show
          Rick Hillegas added a comment - Changing the description of this bug since experiments show that it is more general. The triggered action seems to expect a row which just contains the columns necessary to execute the action. However, if the UPDATE touches columns which aren't mentioned by the trigger, those columns turn up in the row passed to the action. These extra columns can be in the middle of the action row. This throws off the column number of the compiled action and springs this bug.
          Hide
          Rick Hillegas added a comment -

          Attaching a second version of the triggerBug.sql repro. This does not need the procedure class because it demonstrates the bug using an INSERT action. The repro shows that the bug occurs when an extra column is set by the UPDATE statement.

          Show
          Rick Hillegas added a comment - Attaching a second version of the triggerBug.sql repro. This does not need the procedure class because it demonstrates the bug using an INSERT action. The repro shows that the bug occurs when an extra column is set by the UPDATE statement.
          Hide
          Rick Hillegas added a comment -

          It turns out that this bug can manifest itself as data corruptions. I will attach a repro showing that. The bug does not appear in 10.6.2.1 but it does appear in 10.7.1.1. Changing the title of this issue and marking it as a data corrupting regression.

          Show
          Rick Hillegas added a comment - It turns out that this bug can manifest itself as data corruptions. I will attach a repro showing that. The bug does not appear in 10.6.2.1 but it does appear in 10.7.1.1. Changing the title of this issue and marking it as a data corrupting regression.
          Hide
          Rick Hillegas added a comment -

          Attaching triggeredCorruption.sql, a script which demonstrates how this bug can corrupt data.

          Show
          Rick Hillegas added a comment - Attaching triggeredCorruption.sql, a script which demonstrates how this bug can corrupt data.
          Hide
          Rick Hillegas added a comment -

          Through binary search I have identified the checkin which introduced this data corruption:

          r956763 | mamta | 2010-06-21 19:41:48 -0700 (Mon, 21 Jun 2010) | 5 lines

          DERBY-1482

          Only read the needed columns for the update trigger when create trigger has identified the columns which will be used in trigger action through the REFERENCING clause. This will avoid reading columns that are not needed thus avoiding OOM problems if the underlying table has large LOBs.

          Show
          Rick Hillegas added a comment - Through binary search I have identified the checkin which introduced this data corruption: r956763 | mamta | 2010-06-21 19:41:48 -0700 (Mon, 21 Jun 2010) | 5 lines DERBY-1482 Only read the needed columns for the update trigger when create trigger has identified the columns which will be used in trigger action through the REFERENCING clause. This will avoid reading columns that are not needed thus avoiding OOM problems if the underlying table has large LOBs.
          Hide
          Rick Hillegas added a comment -

          Thanks for picking up this issue, Mamta. I am raising the urgency to Blocker. I feel that this issue will affect a lot of applications when they upgrade to 10.7 or 10.8. Thanks.

          Show
          Rick Hillegas added a comment - Thanks for picking up this issue, Mamta. I am raising the urgency to Blocker. I feel that this issue will affect a lot of applications when they upgrade to 10.7 or 10.8. Thanks.
          Hide
          Mamta A. Satoor added a comment -

          Frist of all, thanks so much for having such an easy reproducible test case.

          I looked at the generated code for the trigger action and see the problem Rick described. The purpose of DERBY-1482 was to avoid reading the columns that are not needed by the trigger action, especially the LOB columns which could result in OOM problems. The code that I added for DERBY-1482 looked at the columns referenced in trigger action and assumed that the resultset would have just those columns during the trigger execution time. So, in following example, the trigger action references only 2 out of the 3 columns, the 1st and the 3rd column. The trigger action thus assumes that the resultset would have just columns 1 and 3 in the resultset's column positions 1 and 2. But it did not take into account where the triggering SQL might need additional columns of it's own. In the given example, the triggering UPDATE statement is using the column in position 2 from the trigger table and this causes the trigger action to start using the columns in incorrect column positions.

          I understand the urgency of this jira and based on that, I will see if I can disable the optimization introduced by DERBY-1482. Once that is done, I will look into what could be other ways to solve the unnecessary column reads algorithm. One thing that comes to my mind is to may be read all the columns upto the highest column position required by the trigger action. So in our eg, triggeredCorruption.sql, the trigger action is looking at column positions 1 and 3 and hence we should read columns 1, 2 and 3 even though the trigger action does not need the column 2. Any columns after the highest column position required by the trigger action will be read if the triggering SQL requires it but it will not affect the column positions getting used inside the trigger action.

          I will also think of any other possible way to tackle the column reading optimization. If anyone has any other ideas, please let me know.

          Show
          Mamta A. Satoor added a comment - Frist of all, thanks so much for having such an easy reproducible test case. I looked at the generated code for the trigger action and see the problem Rick described. The purpose of DERBY-1482 was to avoid reading the columns that are not needed by the trigger action, especially the LOB columns which could result in OOM problems. The code that I added for DERBY-1482 looked at the columns referenced in trigger action and assumed that the resultset would have just those columns during the trigger execution time. So, in following example, the trigger action references only 2 out of the 3 columns, the 1st and the 3rd column. The trigger action thus assumes that the resultset would have just columns 1 and 3 in the resultset's column positions 1 and 2. But it did not take into account where the triggering SQL might need additional columns of it's own. In the given example, the triggering UPDATE statement is using the column in position 2 from the trigger table and this causes the trigger action to start using the columns in incorrect column positions. I understand the urgency of this jira and based on that, I will see if I can disable the optimization introduced by DERBY-1482 . Once that is done, I will look into what could be other ways to solve the unnecessary column reads algorithm. One thing that comes to my mind is to may be read all the columns upto the highest column position required by the trigger action. So in our eg, triggeredCorruption.sql, the trigger action is looking at column positions 1 and 3 and hence we should read columns 1, 2 and 3 even though the trigger action does not need the column 2. Any columns after the highest column position required by the trigger action will be read if the triggering SQL requires it but it will not affect the column positions getting used inside the trigger action. I will also think of any other possible way to tackle the column reading optimization. If anyone has any other ideas, please let me know.
          Hide
          Mike Matrigali added a comment -

          at the trigger execution time is there no way to determine the column position in the table vs the column position in the row of the update result set? If I understand the problem the current code is correctly indicating to the update which columns are necessary, we are just accessing the wrong ones. Seems
          like there just needs to be some column fixup or different logic in column accessing for the trigger execution code.

          Show
          Mike Matrigali added a comment - at the trigger execution time is there no way to determine the column position in the table vs the column position in the row of the update result set? If I understand the problem the current code is correctly indicating to the update which columns are necessary, we are just accessing the wrong ones. Seems like there just needs to be some column fixup or different logic in column accessing for the trigger execution code.
          Hide
          Rick Hillegas added a comment -

          It looks to me as though the row set which is passed to the trigger action is already wrapped in another ResultSet. In the case of row-at-a-time triggers, this is a TemporaryRowHolderResultSet. It may be attractive to put some column remapping logic in the TemporaryRowHolderResultSet. I haven't looked at the end-of-statement triggers, but I suspect that there's a wrapper ResultSet there too and some more column remapping logic could be put there. Extra credit if the remapping logic could be factored out and shared.

          Show
          Rick Hillegas added a comment - It looks to me as though the row set which is passed to the trigger action is already wrapped in another ResultSet. In the case of row-at-a-time triggers, this is a TemporaryRowHolderResultSet. It may be attractive to put some column remapping logic in the TemporaryRowHolderResultSet. I haven't looked at the end-of-statement triggers, but I suspect that there's a wrapper ResultSet there too and some more column remapping logic could be put there. Extra credit if the remapping logic could be factored out and shared.
          Hide
          Rick Hillegas added a comment -

          Attaching triggeredBug2.sql. This adds a test for a related case involving statement-scope (rather than row-scope) triggers. The problem does not seem to affect statement-scope triggers, at least not in this test case. I see that in both the statement-scope and row-scope cases, the row passed to the trigger is wrapped in a TemporaryRowHolderResultSet.

          Show
          Rick Hillegas added a comment - Attaching triggeredBug2.sql. This adds a test for a related case involving statement-scope (rather than row-scope) triggers. The problem does not seem to affect statement-scope triggers, at least not in this test case. I see that in both the statement-scope and row-scope cases, the row passed to the trigger is wrapped in a TemporaryRowHolderResultSet.
          Hide
          Mamta A. Satoor added a comment -

          This data corruption regression is in 10.8 and 10.7 codelines.

          I should have a patch for review soon today which will disable the column reading optimization and hence any newly created triggers will work fine. There will be still be issues with triggers already created. This can be a problem with 10.7 since we may already have customers using that release. Fortunately, 10.8 is not out yet to impact any customers. For 10.7, the users will have to drop and recreate their triggers(only the triggers which are defined at row level and which use REFERENCING clause. All the other triggers will work fine).

          Once we have the patch reviewed and committed, I would like to see how I can do the selective column reading and correct column mapping to the resultset at runtime. With the buggy 10.7 and 10.8 code, at the create trigger time, I do the column mapping for the trigger action columns making the assumptions that those columns are the only columns which will exist in the runtime resultset. That assumption is incorrect as shown in the test cases attached to this jira. I am thinking of pursuing the solution where the trigger action sql should not be generated at the create trigger time. Instead, it should be generated when the trigger gets fired and the column position mapping of trigger action columns should happen based on what columns exist in what positions in the runtime resultset. Please let me know if anyone sees a problem with this approach. Any existing generated trigger action sql for existing triggers will be disregarded and it will be just generated at the trigger firing time. This approach will take care of the existing triggers and the new triggers. Once we have this fix in, the 10.7 users will not have to drop and recreate their triggers.

          Show
          Mamta A. Satoor added a comment - This data corruption regression is in 10.8 and 10.7 codelines. I should have a patch for review soon today which will disable the column reading optimization and hence any newly created triggers will work fine. There will be still be issues with triggers already created. This can be a problem with 10.7 since we may already have customers using that release. Fortunately, 10.8 is not out yet to impact any customers. For 10.7, the users will have to drop and recreate their triggers(only the triggers which are defined at row level and which use REFERENCING clause. All the other triggers will work fine). Once we have the patch reviewed and committed, I would like to see how I can do the selective column reading and correct column mapping to the resultset at runtime. With the buggy 10.7 and 10.8 code, at the create trigger time, I do the column mapping for the trigger action columns making the assumptions that those columns are the only columns which will exist in the runtime resultset. That assumption is incorrect as shown in the test cases attached to this jira. I am thinking of pursuing the solution where the trigger action sql should not be generated at the create trigger time. Instead, it should be generated when the trigger gets fired and the column position mapping of trigger action columns should happen based on what columns exist in what positions in the runtime resultset. Please let me know if anyone sees a problem with this approach. Any existing generated trigger action sql for existing triggers will be disregarded and it will be just generated at the trigger firing time. This approach will take care of the existing triggers and the new triggers. Once we have this fix in, the 10.7 users will not have to drop and recreate their triggers.
          Hide
          Mamta A. Satoor added a comment -

          Rick, this data corruption should only occur with row level triggers which are using columns in trigger action through REFERENCING clause. All the other kinds of triggers including statement level triggers should not be impacted.

          I will soon submit a patch for review which will have the test cases provided by you in triggeredBug2.sql.

          Show
          Mamta A. Satoor added a comment - Rick, this data corruption should only occur with row level triggers which are using columns in trigger action through REFERENCING clause. All the other kinds of triggers including statement level triggers should not be impacted. I will soon submit a patch for review which will have the test cases provided by you in triggeredBug2.sql.
          Hide
          Rick Hillegas added a comment -

          Thanks, Mamta. This sounds like good progress. I don't understand the performance implications of compiling the trigger action every time a firing statement is compiled. I suspect that the actions are pre-compiled because someone believed that would yield a performance boost. At a high level, I like the idea of compiling the trigger action along with the firing statement, if only because it gets us out of the business of asking people to drop and recreate their triggers when we correct problems like this. Ever since my stint at Sybase, I have not been a big fan of stored prepared statements--I believe they are a runtime optimization with a thousand brittle consequences.

          At a minimum we would want to make sure that this approach does not break recompilations of prepared statements which may happen because of table changes. For instance, I am thinking about what happens when someone alters the table which is the target of the trigger action.

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - Thanks, Mamta. This sounds like good progress. I don't understand the performance implications of compiling the trigger action every time a firing statement is compiled. I suspect that the actions are pre-compiled because someone believed that would yield a performance boost. At a high level, I like the idea of compiling the trigger action along with the firing statement, if only because it gets us out of the business of asking people to drop and recreate their triggers when we correct problems like this. Ever since my stint at Sybase, I have not been a big fan of stored prepared statements--I believe they are a runtime optimization with a thousand brittle consequences. At a minimum we would want to make sure that this approach does not break recompilations of prepared statements which may happen because of table changes. For instance, I am thinking about what happens when someone alters the table which is the target of the trigger action. Thanks, -Rick
          Hide
          Mamta A. Satoor added a comment - - edited

          Th attached patch(derby5121_patch1_diff.txt) will disable the selective column reading for row level triggers which was introduced by DERBY-1482. As a result of this, Derby will be required to read all the columns from the trigger table. The generated trigger action sql's columns will refer to the columns using theie actual column positions in the trigger table. I have disabled the selective colunm reading by simply assuming in the create trigger code that we are dealing with pre-10.7 database(as if we are in soft upgrade mode from pre-10.7). During the soft-upgrade mode with 10.7 and higher releases, we do not do column reading optimization because the system tables in pre-10.7 are not equipped to keep additional information about trigger action columns.

          This code change was done in DataDictionaryImpl.getTriggerActionString and looks as follows
          boolean in10_7_orHigherVersion = false;
          In addition to the above change, I also had to catch the column missing error in this same method. This can happen if ALTER TABLE DROP COLUMN is dropping a column from the trigger table and that column is getting referenced in the trigger action sql. This scenario currently in the 10.7 and 10.8 codelines get caught when we find that the column being dropped is getting used in trigger action's referenced column list and if so, then we go ahead and drop that trigger if we are doing alter table drop column cascade or we throw an error for trigger dependency if the alter table drop column restrict is being performed. But since with this patch, we do not keep the trigger action's referenced column list anymore, we can't catch the drop column's dependency in the trigger action's referenced column list. Because of this, I have to see if the trigger action column is not found, then I should throw a column not found exception. The catch of that exception will drop the trigger is we are dealing with alter table drop column cascade or it will throw a trigger dependency exception if we are dealing with alter table drop column restrict.

          In addition to the above 2 changes, I had to make following change in TriggerDescriptor.getActionSPS. The if condition used to be
          if((!actionSPS.isValid() ||
          (actionSPS.getPreparedStatement() == null)) &&
          isRow &&
          referencedColsInTriggerAction != null)
          But now with this patch, we don't maintain the information in list referencedColsInTriggerAction and because of that, the above if would always be false. We want to catch triggers that are using REFERENCING clause and because of this, the new if condition will look as follows
          if((!actionSPS.isValid() ||
          (actionSPS.getPreparedStatement() == null)) &&
          isRow && (referencingOld || referencingNew))

          In addition to the above three changes, I have added new test to incorporate Rick's reproducible case.

          I have not changed the comments in the code yet because I hope to work on the fix that I proposed earlier in the jira. Once I have that fix in, I can go ahead and change the comments to match that fix.

          derbyall and junit suite ran fine with my changes. If no one has any comments, I can go ahead and checkin this fix Wednesday evening.

          Show
          Mamta A. Satoor added a comment - - edited Th attached patch(derby5121_patch1_diff.txt) will disable the selective column reading for row level triggers which was introduced by DERBY-1482 . As a result of this, Derby will be required to read all the columns from the trigger table. The generated trigger action sql's columns will refer to the columns using theie actual column positions in the trigger table. I have disabled the selective colunm reading by simply assuming in the create trigger code that we are dealing with pre-10.7 database(as if we are in soft upgrade mode from pre-10.7). During the soft-upgrade mode with 10.7 and higher releases, we do not do column reading optimization because the system tables in pre-10.7 are not equipped to keep additional information about trigger action columns. This code change was done in DataDictionaryImpl.getTriggerActionString and looks as follows boolean in10_7_orHigherVersion = false; In addition to the above change, I also had to catch the column missing error in this same method. This can happen if ALTER TABLE DROP COLUMN is dropping a column from the trigger table and that column is getting referenced in the trigger action sql. This scenario currently in the 10.7 and 10.8 codelines get caught when we find that the column being dropped is getting used in trigger action's referenced column list and if so, then we go ahead and drop that trigger if we are doing alter table drop column cascade or we throw an error for trigger dependency if the alter table drop column restrict is being performed. But since with this patch, we do not keep the trigger action's referenced column list anymore, we can't catch the drop column's dependency in the trigger action's referenced column list. Because of this, I have to see if the trigger action column is not found, then I should throw a column not found exception. The catch of that exception will drop the trigger is we are dealing with alter table drop column cascade or it will throw a trigger dependency exception if we are dealing with alter table drop column restrict. In addition to the above 2 changes, I had to make following change in TriggerDescriptor.getActionSPS. The if condition used to be if((!actionSPS.isValid() || (actionSPS.getPreparedStatement() == null)) && isRow && referencedColsInTriggerAction != null) But now with this patch, we don't maintain the information in list referencedColsInTriggerAction and because of that, the above if would always be false. We want to catch triggers that are using REFERENCING clause and because of this, the new if condition will look as follows if((!actionSPS.isValid() || (actionSPS.getPreparedStatement() == null)) && isRow && (referencingOld || referencingNew)) In addition to the above three changes, I have added new test to incorporate Rick's reproducible case. I have not changed the comments in the code yet because I hope to work on the fix that I proposed earlier in the jira. Once I have that fix in, I can go ahead and change the comments to match that fix. derbyall and junit suite ran fine with my changes. If no one has any comments, I can go ahead and checkin this fix Wednesday evening.
          Hide
          Mamta A. Satoor added a comment -

          This patch is based on 10.7 codeline.

          Show
          Mamta A. Satoor added a comment - This patch is based on 10.7 codeline.
          Hide
          Rick Hillegas added a comment -

          Thanks for the patch, Mamta. I can verify that this patch causes the test case to succeed. I believe there is no value in testing this on the production system where the original problem surfaced. That is because that system uses a 10.7.1.0 database. Once we have a fix which works on soft-upgrade from 10.7.1.0 I will run some tests on that production system. Thanks.

          Show
          Rick Hillegas added a comment - Thanks for the patch, Mamta. I can verify that this patch causes the test case to succeed. I believe there is no value in testing this on the production system where the original problem surfaced. That is because that system uses a 10.7.1.0 database. Once we have a fix which works on soft-upgrade from 10.7.1.0 I will run some tests on that production system. Thanks.
          Hide
          Mike Matrigali added a comment -

          I know you said you were not "fixing" the existing comments, but I believe the following change is worth a comment to at least associated it with this issue, and indicating this is the key place that is disabling previous work to not fetch unneeded columns:

          • boolean in10_7_orHigherVersion =
          • checkVersion(DataDictionary.DD_VERSION_DERBY_10_7,null);
            + boolean in10_7_orHigherVersion = false;
          Show
          Mike Matrigali added a comment - I know you said you were not "fixing" the existing comments, but I believe the following change is worth a comment to at least associated it with this issue, and indicating this is the key place that is disabling previous work to not fetch unneeded columns: boolean in10_7_orHigherVersion = checkVersion(DataDictionary.DD_VERSION_DERBY_10_7,null); + boolean in10_7_orHigherVersion = false;
          Hide
          Mamta A. Satoor added a comment -

          I am working on backporting this simple fix to disbale the column read optimization from 10.7 to trunk but I am seeing OOM errors for a subset of org.apache.derbyTesting.functionTests.tests.memory.TriggerTests.

          The OOM should depend on how much heap the test gets run with. On both 10.7 and trunk(with 10.7 changes backported). I see no failures if I run the test with higher heap as shown below
          java -Dderby.tests.trace=true -Xmx256M -XX:MaxPermSize=128M junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.memory.TriggerTests

          But if I run the test on both 10.7 and trunk(with 10.7 changes backported) with lower heap, I see OOM errors
          java -Dderby.tests.trace=true -Xmx16M -Dderby.storage.pageCacheSize=100 junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.memory.TriggerTests

          So the behavior on the 2 codelines with restricted and generous heap is the same when the test is run directly. But when I run the junit suite as follows, I get OOM errors on trunk but not on 10.7 codeline. At this point, I am not sure why. Is there a way that we change the available heap on trunk for org.apache.derbyTesting.functionTests.tests.memory.TriggerTests when it is run through the functionTests.suites.All junit suite? That's the only reason I can think why the test would fail anywere. It appears that on 10.7, we do not change the heap for org.apache.derbyTesting.functionTests.tests.memory.TriggerTests when it is run through the functionTests.suites.All junit suite? Does anyone have any clue about this difference in behavior between 10.7 and trunk? thanks
          java -Dderby.tests.trace=true -Xmx256M -XX:MaxPermSize=128M junit.textui.TestRunner org.apache.derbyTesting.functionTests.suites.All > runall.out 2>&1

          Show
          Mamta A. Satoor added a comment - I am working on backporting this simple fix to disbale the column read optimization from 10.7 to trunk but I am seeing OOM errors for a subset of org.apache.derbyTesting.functionTests.tests.memory.TriggerTests. The OOM should depend on how much heap the test gets run with. On both 10.7 and trunk(with 10.7 changes backported). I see no failures if I run the test with higher heap as shown below java -Dderby.tests.trace=true -Xmx256M -XX:MaxPermSize=128M junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.memory.TriggerTests But if I run the test on both 10.7 and trunk(with 10.7 changes backported) with lower heap, I see OOM errors java -Dderby.tests.trace=true -Xmx16M -Dderby.storage.pageCacheSize=100 junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.memory.TriggerTests So the behavior on the 2 codelines with restricted and generous heap is the same when the test is run directly. But when I run the junit suite as follows, I get OOM errors on trunk but not on 10.7 codeline. At this point, I am not sure why. Is there a way that we change the available heap on trunk for org.apache.derbyTesting.functionTests.tests.memory.TriggerTests when it is run through the functionTests.suites.All junit suite? That's the only reason I can think why the test would fail anywere. It appears that on 10.7, we do not change the heap for org.apache.derbyTesting.functionTests.tests.memory.TriggerTests when it is run through the functionTests.suites.All junit suite? Does anyone have any clue about this difference in behavior between 10.7 and trunk? thanks java -Dderby.tests.trace=true -Xmx256M -XX:MaxPermSize=128M junit.textui.TestRunner org.apache.derbyTesting.functionTests.suites.All > runall.out 2>&1
          Hide
          Myrna van Lunteren added a comment -

          Is it possible a test got added in trunk that sets memory settings but doesn't set them back?
          Do you see this behavior only with suites.All, or also with memory._Suite ?
          I sometimes use a little non-checked in TempSuite class that basically similates AllPackages (called from suites.All)
          but leaves sections out, to find offending tests...

          Show
          Myrna van Lunteren added a comment - Is it possible a test got added in trunk that sets memory settings but doesn't set them back? Do you see this behavior only with suites.All, or also with memory._Suite ? I sometimes use a little non-checked in TempSuite class that basically similates AllPackages (called from suites.All) but leaves sections out, to find offending tests...
          Hide
          Mamta A. Satoor added a comment -

          Myrna, thanks for taking the time go over the OOM problem. I will try to narrow down the suites to see where things might be going wrong on trunk.

          BTW, running memory._Suite by itself does not run into OOM.

          Show
          Mamta A. Satoor added a comment - Myrna, thanks for taking the time go over the OOM problem. I will try to narrow down the suites to see where things might be going wrong on trunk. BTW, running memory._Suite by itself does not run into OOM.
          Hide
          Kathey Marsden added a comment -

          Hi Mamta, for the IBM JVM's there is a tool tool that you can use to analyze heap dumps and memory issues , the .phd file that is generated when you get an out of memory.
          http://www.alphaworks.ibm.com/tech/heapanalyzer

          You run it like java -Xmx1200m -jar ha21.jar <phd file>

          And in a box that comes up on the first screen it shows the heap size. The menu options allow you to identify leak suspects and it has other features.

          I don't know how the heap size could be changed at runtime though.

          Show
          Kathey Marsden added a comment - Hi Mamta, for the IBM JVM's there is a tool tool that you can use to analyze heap dumps and memory issues , the .phd file that is generated when you get an out of memory. http://www.alphaworks.ibm.com/tech/heapanalyzer You run it like java -Xmx1200m -jar ha21.jar <phd file> And in a box that comes up on the first screen it shows the heap size. The menu options allow you to identify leak suspects and it has other features. I don't know how the heap size could be changed at runtime though.
          Hide
          Mamta A. Satoor added a comment -

          I am debugging the code to see what happens at the execution time of trigger firing. Rick, you mentioned TemporaryRowHolderResultSet in one of your comments. I will debug that further but do you know offhand if each trigger gets it's own copy of TemporaryRowHolderResultSet. If so, then I can probably maddle with how column mapping happens. This mapping can be different for different triggers on the same table and I didn't want to mess up TemporaryRowHolderResultSet if it is being shared by all the triggers on the table which are going to get fired. Thanks

          Show
          Mamta A. Satoor added a comment - I am debugging the code to see what happens at the execution time of trigger firing. Rick, you mentioned TemporaryRowHolderResultSet in one of your comments. I will debug that further but do you know offhand if each trigger gets it's own copy of TemporaryRowHolderResultSet. If so, then I can probably maddle with how column mapping happens. This mapping can be different for different triggers on the same table and I didn't want to mess up TemporaryRowHolderResultSet if it is being shared by all the triggers on the table which are going to get fired. Thanks
          Hide
          Rick Hillegas added a comment -

          Hi Mamta,

          It appears to me that each trigger gets its own TemporarayRowHolderResultSet. See the calls to getNewRSOnCurrentRow(). Thanks.

          Show
          Rick Hillegas added a comment - Hi Mamta, It appears to me that each trigger gets its own TemporarayRowHolderResultSet. See the calls to getNewRSOnCurrentRow(). Thanks.
          Hide
          Rick Hillegas added a comment -

          Attaching Test_5121.java. This test exhaustively walks through all subsets and permutations of columns for a trigger which inserts into a side table based on updates to a master table. The test runs cleanly with Mamta's patch applied. I think this will be a good acceptance test for the solution. Here is how you run this test:

          Usage:

          java Test_5121 connectionURL columnCount debug

          where

          connectionURL is the url to the database
          columnCount is the number of columns in the triggering table
          debug indicates whether to print out extra debug information

          E.g.:

          java Test_5121 "jdbc:derby:memory:db;create=true" 3 false

          Show
          Rick Hillegas added a comment - Attaching Test_5121.java. This test exhaustively walks through all subsets and permutations of columns for a trigger which inserts into a side table based on updates to a master table. The test runs cleanly with Mamta's patch applied. I think this will be a good acceptance test for the solution. Here is how you run this test: Usage: java Test_5121 connectionURL columnCount debug where connectionURL is the url to the database columnCount is the number of columns in the triggering table debug indicates whether to print out extra debug information E.g.: java Test_5121 "jdbc:derby:memory:db;create=true" 3 false
          Hide
          Mamta A. Satoor added a comment -

          Rick, your test will be very useful with the code changes I am making in the my codeline. When I have the patch ready I will add these tests as part of our junit test harness.

          Show
          Mamta A. Satoor added a comment - Rick, your test will be very useful with the code changes I am making in the my codeline. When I have the patch ready I will add these tests as part of our junit test harness.
          Hide
          Mamta A. Satoor added a comment -

          I just wanted to summarize the problem here to make it easier to understand the issue and the solution I am working on.

          Some background information : With DERBY-1482, we decided to read only the columns really needed during trigger execution. Trigger only knows about trigger columns and columns in it's trigger action used through the REFERENCING clause. And so DERBY-1482 made the assumption that those will be the only columns read from the trigger table and it uses the relative column positions in that resultset to access the columns in it's trigger action when we generate the internal sql for the trigger action. The problem is that there can be cases when the SQL causing the trigger to fire needs to read more columns than just what the trigger needs. eg
          create table t1( id int, name varchar( 50 ) );
          create table t2
          (
          name varchar( 50 ) not null,
          description int not null,
          id integer
          );
          insert into t2( name, description ) values ( 'Foo Name', 0 );

          create trigger t2UpdateTrigger
          after UPDATE of name
          on t2
          referencing
          new row as nr
          for each ROW
          insert into t1 values ( nr.id, nr.name );

          The trigger above only needs columns "name" and "id" from the trigger table and hence it will assume that the runtime resultset will have just those 2 columns, "name" as first column in the resultset and "id" as the 2nd column in the resultset and using that assumption, it will change trigger action sql "insert into t1 values ( nr.id, nr.name )" to following "insert into t1 values ( CAST(org.apache.derby.iapi.db.Factory::getTriggerExecutionContext().getNewRow().getObject(2) AS INTEGER) , CAST (org.apache.derby.iapi.db.Factory::getTriggerExecutionContext().getNewRow().getObject(1) AS VARCHAR(50)) )

          But the following triggering sql needs more columns than just "name" and "id", it also needs "description".(The list of columns required by triggering sql are columns used by the triggering sql and columns needed by the triggers which will get fired).
          update t2 set name = 'Another name' , description = 1;
          So the runtime resulset will end up having columns "name", "description" and "id". So the column "id" is the 3rd column in the resultset and not 2nd column in the resultset as expected by the trigger.

          The solution I am working on is to see if we can map out only the columns needed by the trigger from the actual runtime resulset created by the triggering sql.

          Show
          Mamta A. Satoor added a comment - I just wanted to summarize the problem here to make it easier to understand the issue and the solution I am working on. Some background information : With DERBY-1482 , we decided to read only the columns really needed during trigger execution. Trigger only knows about trigger columns and columns in it's trigger action used through the REFERENCING clause. And so DERBY-1482 made the assumption that those will be the only columns read from the trigger table and it uses the relative column positions in that resultset to access the columns in it's trigger action when we generate the internal sql for the trigger action. The problem is that there can be cases when the SQL causing the trigger to fire needs to read more columns than just what the trigger needs. eg create table t1( id int, name varchar( 50 ) ); create table t2 ( name varchar( 50 ) not null, description int not null, id integer ); insert into t2( name, description ) values ( 'Foo Name', 0 ); create trigger t2UpdateTrigger after UPDATE of name on t2 referencing new row as nr for each ROW insert into t1 values ( nr.id, nr.name ); The trigger above only needs columns "name" and "id" from the trigger table and hence it will assume that the runtime resultset will have just those 2 columns, "name" as first column in the resultset and "id" as the 2nd column in the resultset and using that assumption, it will change trigger action sql "insert into t1 values ( nr.id, nr.name )" to following "insert into t1 values ( CAST(org.apache.derby.iapi.db.Factory::getTriggerExecutionContext().getNewRow().getObject(2) AS INTEGER) , CAST (org.apache.derby.iapi.db.Factory::getTriggerExecutionContext().getNewRow().getObject(1) AS VARCHAR(50)) ) But the following triggering sql needs more columns than just "name" and "id", it also needs "description".(The list of columns required by triggering sql are columns used by the triggering sql and columns needed by the triggers which will get fired). update t2 set name = 'Another name' , description = 1; So the runtime resulset will end up having columns "name", "description" and "id". So the column "id" is the 3rd column in the resultset and not 2nd column in the resultset as expected by the trigger. The solution I am working on is to see if we can map out only the columns needed by the trigger from the actual runtime resulset created by the triggering sql.
          Hide
          Mike Matrigali added a comment -

          Are you still planning on first backporting the fix made to 10.7 into the trunk line? Have you done any more work to determine if the out of memory is this change or something in your environment? Given the issues we have had with triggers it may make more sense to go with the safe change for 10.8 and then later after 10.8 is cut you can check in your new fix to trunk and get testing there before putting it into a released codeline.

          My interpretation of reviewing the change is that it should be safe and not cause unexpected out of memory. It is basically just
          making all code go through the existing code path that all pre 10.7 databases are already executing.

          If you are uncomfortable with your current testing maybe you could post the exact patch against trunk that you are trying and someone else can run tests based on it.

          Show
          Mike Matrigali added a comment - Are you still planning on first backporting the fix made to 10.7 into the trunk line? Have you done any more work to determine if the out of memory is this change or something in your environment? Given the issues we have had with triggers it may make more sense to go with the safe change for 10.8 and then later after 10.8 is cut you can check in your new fix to trunk and get testing there before putting it into a released codeline. My interpretation of reviewing the change is that it should be safe and not cause unexpected out of memory. It is basically just making all code go through the existing code path that all pre 10.7 databases are already executing. If you are uncomfortable with your current testing maybe you could post the exact patch against trunk that you are trying and someone else can run tests based on it.
          Hide
          Mamta A. Satoor added a comment -

          I am attaching the patch for trunk which is trying to backport changes made to 10.7 to backout DERBY-1482 changes. But these changes on trunk cause a subset of trigger tests to run out of memory. Maybe someone else can try running junit suite if they have the cycle to see if they see the problem too.

          Show
          Mamta A. Satoor added a comment - I am attaching the patch for trunk which is trying to backport changes made to 10.7 to backout DERBY-1482 changes. But these changes on trunk cause a subset of trigger tests to run out of memory. Maybe someone else can try running junit suite if they have the cycle to see if they see the problem too.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-5121-01-aa-runTimeRemapping.diff. This patch remaps columns at run time in order to fix the bug. I think this at least demonstrates the feasibility of column remapping. An improvement on this patch would be to calculate the remapping at compile time rather than run time.

          Some trickiness is involved because some of the compiled column maps are 0-based and others are 1-based. I have run Test_5121 on this patch and it passes cleanly for 1, 2, 3, and 4 column tables. I stopped running experiments then because the 4 column table gave rise to 61440 test cases.

          I will run the full regression suite to see if that shows any problems.

          Touches the following files:

          M java/engine/org/apache/derby/impl/sql/execute/TemporaryRowHolderResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/InsertResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/GenericTriggerExecutor.java
          M java/engine/org/apache/derby/impl/sql/execute/TriggerEventActivator.java
          M java/engine/org/apache/derby/impl/sql/execute/StatementTriggerExecutor.java
          M java/engine/org/apache/derby/impl/sql/execute/DeleteResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/RowTriggerExecutor.java
          M java/engine/org/apache/derby/impl/sql/execute/UpdateResultSet.java

          Show
          Rick Hillegas added a comment - Attaching derby-5121-01-aa-runTimeRemapping.diff. This patch remaps columns at run time in order to fix the bug. I think this at least demonstrates the feasibility of column remapping. An improvement on this patch would be to calculate the remapping at compile time rather than run time. Some trickiness is involved because some of the compiled column maps are 0-based and others are 1-based. I have run Test_5121 on this patch and it passes cleanly for 1, 2, 3, and 4 column tables. I stopped running experiments then because the 4 column table gave rise to 61440 test cases. I will run the full regression suite to see if that shows any problems. Touches the following files: M java/engine/org/apache/derby/impl/sql/execute/TemporaryRowHolderResultSet.java M java/engine/org/apache/derby/impl/sql/execute/InsertResultSet.java M java/engine/org/apache/derby/impl/sql/execute/GenericTriggerExecutor.java M java/engine/org/apache/derby/impl/sql/execute/TriggerEventActivator.java M java/engine/org/apache/derby/impl/sql/execute/StatementTriggerExecutor.java M java/engine/org/apache/derby/impl/sql/execute/DeleteResultSet.java M java/engine/org/apache/derby/impl/sql/execute/RowTriggerExecutor.java M java/engine/org/apache/derby/impl/sql/execute/UpdateResultSet.java
          Hide
          Mike Matrigali added a comment -

          I believe the tests that you see failing in your run were written specifically to test
          DERBY-1482. In order to test that blobs are not being read into memory the tests specifically are coded
          to read blobs into memory in the cases that DERBY-1482 fixes. It is ASSUMED that in the normal suite run
          that there will be enough memory for the tests to pass, and that when the same tests are run as part of the
          "small memory" test suite that the bugs will be caught. But maybe due to timing, machine resources, or
          previous tests run these tests no longer successfully run guaranteed in default suite run on trunk. It would
          seem like disabling DERBY-1482 should also disable the memory specific tests if we think those tests are
          not valid once the feature is disabled.

          Show
          Mike Matrigali added a comment - I believe the tests that you see failing in your run were written specifically to test DERBY-1482 . In order to test that blobs are not being read into memory the tests specifically are coded to read blobs into memory in the cases that DERBY-1482 fixes. It is ASSUMED that in the normal suite run that there will be enough memory for the tests to pass, and that when the same tests are run as part of the "small memory" test suite that the bugs will be caught. But maybe due to timing, machine resources, or previous tests run these tests no longer successfully run guaranteed in default suite run on trunk. It would seem like disabling DERBY-1482 should also disable the memory specific tests if we think those tests are not valid once the feature is disabled.
          Hide
          Mamta A. Satoor added a comment -

          I am attaching a new patch (DERBY_5121_backoutDerby1492_changes_patch3_trunk_diff.txt) for trunk which will backport DERBY-1482 changes from trunk, same as 10.7 and will do some additional work(this patch has additional changes than DERBY_5121_patch2_trunk_diff.txt).

          The patch will also disable the tests that were added for DERBY-1482.

          With DERBY-1482, these tests would not read in large object columns into memory because the triggers didn't need them. But now that DERBY-1482 changes are being backed out, the large object columns will be read in which can cause the test to run out of memory depending on how much heap is available to it.

          I am running the junit suite just one more time to make sure that I don't run into any OOMs. Once the junit tests run fine, I will go ahead and commit this patch to trunk, thus backing out DERBY-1482 changes.

          The patch also has a comment in DataDictionaryImpl:getTriggerActionString explaining the code changes for backout. I will add that comment in 10.7 too.

          Once the tests run fine on trunk, I will disable DERBY-1482 tests on 10.7 too.

          If any comments, please feel free to post them.

          Show
          Mamta A. Satoor added a comment - I am attaching a new patch (DERBY_5121_backoutDerby1492_changes_patch3_trunk_diff.txt) for trunk which will backport DERBY-1482 changes from trunk, same as 10.7 and will do some additional work(this patch has additional changes than DERBY_5121_patch2_trunk_diff.txt). The patch will also disable the tests that were added for DERBY-1482 . With DERBY-1482 , these tests would not read in large object columns into memory because the triggers didn't need them. But now that DERBY-1482 changes are being backed out, the large object columns will be read in which can cause the test to run out of memory depending on how much heap is available to it. I am running the junit suite just one more time to make sure that I don't run into any OOMs. Once the junit tests run fine, I will go ahead and commit this patch to trunk, thus backing out DERBY-1482 changes. The patch also has a comment in DataDictionaryImpl:getTriggerActionString explaining the code changes for backout. I will add that comment in 10.7 too. Once the tests run fine on trunk, I will disable DERBY-1482 tests on 10.7 too. If any comments, please feel free to post them.
          Hide
          Mamta A. Satoor added a comment -

          Hi Rick, thanks for the patch. BTW, I have a patch too to do the remapping of the columns. It is lacking comments at this point. I will work on adding comments so it is easier to understand. I ran your extensive tests with my changes and the tests worked fine. I haven't finished running the junit and derbyall suite with it yet, junit suite is running right now. I hope to put a patch with comments sometime soon today.

          Show
          Mamta A. Satoor added a comment - Hi Rick, thanks for the patch. BTW, I have a patch too to do the remapping of the columns. It is lacking comments at this point. I will work on adding comments so it is easier to understand. I ran your extensive tests with my changes and the tests worked fine. I haven't finished running the junit and derbyall suite with it yet, junit suite is running right now. I hope to put a patch with comments sometime soon today.
          Hide
          Rick Hillegas added a comment -

          The derby-5121-01-aa-runTimeRemapping.diff patch creates a shower of problems in the regression tests.

          Show
          Rick Hillegas added a comment - The derby-5121-01-aa-runTimeRemapping.diff patch creates a shower of problems in the regression tests.
          Hide
          Rick Hillegas added a comment -

          Thanks, Mamta. Just want to clarify: After you check in your disabling fix, additional work will still be needed to throw away the trigger actions which have been compiled in 10.7 databases? Thanks.

          Show
          Rick Hillegas added a comment - Thanks, Mamta. Just want to clarify: After you check in your disabling fix, additional work will still be needed to throw away the trigger actions which have been compiled in 10.7 databases? Thanks.
          Hide
          Mamta A. Satoor added a comment -

          Hi Rick.

          The backout of DERBY-1482 changes will only take care of of new triggers.

          Unfortunately, the old triggers(created in 10.7) will still have problems and will have to be manually dropped and recreated. The triggers impacted are the row level triggers with REFERENCING clause. Thanks

          Show
          Mamta A. Satoor added a comment - Hi Rick. The backout of DERBY-1482 changes will only take care of of new triggers. Unfortunately, the old triggers(created in 10.7) will still have problems and will have to be manually dropped and recreated. The triggers impacted are the row level triggers with REFERENCING clause. Thanks
          Hide
          Mamta A. Satoor added a comment -

          As a bigger solution to having to ask the users to drop and recreate the triggers during upgrade, it will be ideal if we could internally do this ourselves. I think there are 2 possibilites
          1)mark the SPS associated with trigger actions as invalid at the time of upgrade. This way, when they get used next time, we will recreate the correction trigger action sql
          2)drop and recreate the trigger action SPSs at the time of upgrade. I am not sure though if this will run into privileges/roles issues since the user doing the upgrade may not have permissions to touch the system tables.

          I would like to investigate option 1) and see if it is doable. I will hold on to posting my patch of remapping and will concentrate on option 1) because if the option 1) works than we can avoid having to ask the users to drop and recreate the triggers. Thanks

          Show
          Mamta A. Satoor added a comment - As a bigger solution to having to ask the users to drop and recreate the triggers during upgrade, it will be ideal if we could internally do this ourselves. I think there are 2 possibilites 1)mark the SPS associated with trigger actions as invalid at the time of upgrade. This way, when they get used next time, we will recreate the correction trigger action sql 2)drop and recreate the trigger action SPSs at the time of upgrade. I am not sure though if this will run into privileges/roles issues since the user doing the upgrade may not have permissions to touch the system tables. I would like to investigate option 1) and see if it is doable. I will hold on to posting my patch of remapping and will concentrate on option 1) because if the option 1) works than we can avoid having to ask the users to drop and recreate the triggers. Thanks
          Hide
          Mamta A. Satoor added a comment -

          I looked through the upgrade code and saw that we already mark all the SPS invalid during soft and hard upgrade. This is great because at the time of trigger execution, we check if the trigger is invalid, it is row level and it has REFERENCING clause then we should regenerate the trigger action sql. So, when a user with 10.7.1.1(the version where the triggers can cause data corruption) upgrades to a release which has DERBY-1482 changes backed out (at this moment, that would be 10.8 and next release of 10.7. I will work on bumping the release number of 10,7), the buggy triggers will get fixed and will not cause any data corruption.

          Additionally, once we have the right fix for DERBY-1482 so that the resultset received by the trigger during execution matches trigger's expectation of column numbering, we can introduce column reading from trigger table to only the columns that are really necessary rather than reading all the columns (which is what we do right now after backing out DERBY-1482 changes).

          I have created upgrade test for 10.8 which will test the behavior explained above.

          In other words, the users do not have to drop and recreate their triggers anymore after upgrading to a release which has DERBY-1482 changes backed out.

          If there are any questions, please let me know.

          Show
          Mamta A. Satoor added a comment - I looked through the upgrade code and saw that we already mark all the SPS invalid during soft and hard upgrade. This is great because at the time of trigger execution, we check if the trigger is invalid, it is row level and it has REFERENCING clause then we should regenerate the trigger action sql. So, when a user with 10.7.1.1(the version where the triggers can cause data corruption) upgrades to a release which has DERBY-1482 changes backed out (at this moment, that would be 10.8 and next release of 10.7. I will work on bumping the release number of 10,7), the buggy triggers will get fixed and will not cause any data corruption. Additionally, once we have the right fix for DERBY-1482 so that the resultset received by the trigger during execution matches trigger's expectation of column numbering, we can introduce column reading from trigger table to only the columns that are really necessary rather than reading all the columns (which is what we do right now after backing out DERBY-1482 changes). I have created upgrade test for 10.8 which will test the behavior explained above. In other words, the users do not have to drop and recreate their triggers anymore after upgrading to a release which has DERBY-1482 changes backed out. If there are any questions, please let me know.
          Hide
          Mamta A. Satoor added a comment -

          Attaching the release note for this issue. I will go ahead and close this issue. The work to do selective column reading during trigger execution and providing the correct resultset to trigger at execution time can go as part of DERBY-1482. DERBY-1482 is the original issue to provide this functionality.

          Show
          Mamta A. Satoor added a comment - Attaching the release note for this issue. I will go ahead and close this issue. The work to do selective column reading during trigger execution and providing the correct resultset to trigger at execution time can go as part of DERBY-1482 . DERBY-1482 is the original issue to provide this functionality.
          Hide
          Rick Hillegas added a comment -

          Thanks, Mamta. I'm afraid I can't close this issue yet. I built a quick 10.8.1.0 distribution and ran the production application with it. The good news is that the original error went away. Unfortunately, now I'm getting a new error when the update trigger fires. I will put some effort into scripting the problem. Thanks.

          Show
          Rick Hillegas added a comment - Thanks, Mamta. I'm afraid I can't close this issue yet. I built a quick 10.8.1.0 distribution and ran the production application with it. The good news is that the original error went away. Unfortunately, now I'm getting a new error when the update trigger fires. I will put some effort into scripting the problem. Thanks.
          Hide
          Mamta A. Satoor added a comment -

          Rick, I was hoping that this would fix all the problems since we took out DERBY-1482 changes but looks like not. Can you please post what error you are seeing and I will start poking in the code to see if I can find a reason before you have a repro case? Thanks

          Show
          Mamta A. Satoor added a comment - Rick, I was hoping that this would fix all the problems since we took out DERBY-1482 changes but looks like not. Can you please post what error you are seeing and I will start poking in the code to see if I can find a reason before you have a repro case? Thanks
          Hide
          Rick Hillegas added a comment -

          Attaching a repro for the new problem I am seeing. I haven't had time to trim this down to a smaller case, but I wanted to get this posted quickly Before running this repro, first compile Proc_5121.java. Then do the following:

          1) Using 10.7.1.1, run 5121_create.sql through ij. This will create a 10.7 database with a table which has a bad update trigger. The last statement in the script fails--it is the original statement which disclosed the original problem.

          2) Using the trunk, run 5121_upgrade.sql through ij. You will have to set -Dderby.database.allowPreReleaseUpgrade=true in order to get the soft upgrade to run. The script contains one command, namely, the statement which disclosed the original problem. Now this statement fails with the following error:

          ERROR 38000: The exception 'java.sql.SQLException: Column '6' not found.' was thrown while evaluating an expression.
          ERROR S0022: Column '6' not found.

          Show
          Rick Hillegas added a comment - Attaching a repro for the new problem I am seeing. I haven't had time to trim this down to a smaller case, but I wanted to get this posted quickly Before running this repro, first compile Proc_5121.java. Then do the following: 1) Using 10.7.1.1, run 5121_create.sql through ij. This will create a 10.7 database with a table which has a bad update trigger. The last statement in the script fails--it is the original statement which disclosed the original problem. 2) Using the trunk, run 5121_upgrade.sql through ij. You will have to set -Dderby.database.allowPreReleaseUpgrade=true in order to get the soft upgrade to run. The script contains one command, namely, the statement which disclosed the original problem. Now this statement fails with the following error: ERROR 38000: The exception 'java.sql.SQLException: Column '6' not found.' was thrown while evaluating an expression. ERROR S0022: Column '6' not found.
          Hide
          Mike Matrigali added a comment -

          it might be useful to run this test case where step 2 is executed against main version without the changes submitted for DERBY-5121 . I am wondering if this is a different bug associated with automatic recompile of triggers where the trigger is a procedure call?

          Show
          Mike Matrigali added a comment - it might be useful to run this test case where step 2 is executed against main version without the changes submitted for DERBY-5121 . I am wondering if this is a different bug associated with automatic recompile of triggers where the trigger is a procedure call?
          Hide
          Mamta A. Satoor added a comment -

          Hi Rick, I found the problem behind the failure. This happened because DERBY-1482 wasn't completely backed out. We have backed out the code for the triggers so that now that look for the columns in their actual column positions at execution time. But DERBY-1482 also made changes to UPDATE code to read only the colunms needed by it and the triggers that it is going to fire. We need to backout the changes to UPDATE code to make sure that it reads all the columns from the trigger table and not do selective column reading.

          In the attached patch(based on trunk), I have made that change and run the test you provided and it now runs with no exception. I haven't done any other testing on it. Please feel free to use this patch for testing with the problem 10.7.1.1 database that you are working with.

          I would like to work on following
          1)spend little more time on the DERBY-1482 changes to make sure that I am backing out everything relevant.
          2)run existing junit and derbyall tests once I have accomplished 1)
          3)Add an upgrade test case for the latest example you provided.

          The reason we didn't see this failure with your last test case was that UPDATE sql was coincidentally reading all the columns upto the highest column position required by the trigger and hence all the columns were available in their right positions even though not all the columns may have been read from the trigger table.

          Please let me know if you have any questions. I would also appreciate if you would post on the list how your testing goes with my patch if you find time to do any testing.

          Show
          Mamta A. Satoor added a comment - Hi Rick, I found the problem behind the failure. This happened because DERBY-1482 wasn't completely backed out. We have backed out the code for the triggers so that now that look for the columns in their actual column positions at execution time. But DERBY-1482 also made changes to UPDATE code to read only the colunms needed by it and the triggers that it is going to fire. We need to backout the changes to UPDATE code to make sure that it reads all the columns from the trigger table and not do selective column reading. In the attached patch(based on trunk), I have made that change and run the test you provided and it now runs with no exception. I haven't done any other testing on it. Please feel free to use this patch for testing with the problem 10.7.1.1 database that you are working with. I would like to work on following 1)spend little more time on the DERBY-1482 changes to make sure that I am backing out everything relevant. 2)run existing junit and derbyall tests once I have accomplished 1) 3)Add an upgrade test case for the latest example you provided. The reason we didn't see this failure with your last test case was that UPDATE sql was coincidentally reading all the columns upto the highest column position required by the trigger and hence all the columns were available in their right positions even though not all the columns may have been read from the trigger table. Please let me know if you have any questions. I would also appreciate if you would post on the list how your testing goes with my patch if you find time to do any testing.
          Hide
          Mamta A. Satoor added a comment -

          I have spent some time analyzing the changes that went into UpdateNode as part of DERBY-1482 and I believe that the attached patch, DERBY_5121_NotReadForCommit_patch4_trunk_diff.txt, does the correct job of undoing those changes from UpdateNode. The patch I attached on March 29th(DERBY_5121_NotReadForCommit_patche_trunk_diff.txt) did not remove those changes properly.

          I have also written an upgrade case testing the behavior of UPDATE reading correct columns from the trigger table so that trigger finds the columns it needs.

          I am running junit suite right now to make sure nothing breaks with the attached patch. will also run derbyall suite once junit is finished successfully.

          Show
          Mamta A. Satoor added a comment - I have spent some time analyzing the changes that went into UpdateNode as part of DERBY-1482 and I believe that the attached patch, DERBY_5121_NotReadForCommit_patch4_trunk_diff.txt, does the correct job of undoing those changes from UpdateNode. The patch I attached on March 29th(DERBY_5121_NotReadForCommit_patche_trunk_diff.txt) did not remove those changes properly. I have also written an upgrade case testing the behavior of UPDATE reading correct columns from the trigger table so that trigger finds the columns it needs. I am running junit suite right now to make sure nothing breaks with the attached patch. will also run derbyall suite once junit is finished successfully.
          Hide
          Rick Hillegas added a comment -

          Thanks, Mamta. I have verified that the production problem goes away if I apply DERBY_5121_NotReadForCommit_patch4_trunk_diff.txt. Thanks.

          Show
          Rick Hillegas added a comment - Thanks, Mamta. I have verified that the production problem goes away if I apply DERBY_5121_NotReadForCommit_patch4_trunk_diff.txt. Thanks.
          Hide
          Rick Hillegas added a comment -

          Attaching Test_5121_Upgrade.java. This is a modification of the previous test of column subsets and permutations. This splits the schema creation apart from the DML so that the schema can be created under 10.7 and the UPDATE statements and trigger-firings can be tested under 10.8.

          To run this test, do the following:

          1) Create the schema by setting your classpath to include the 10.7 jars. Delete the db database. Then do the following to create the schema for 3 column tables:

          java Test_5121_Upgrade "jdbc:derby:db;create=true" 3 false

          2) Now set your classpath to include the 10.8 jars. Test trigger-firing after soft-upgrade by doing the following:

          java -Dderby.database.allowPreReleaseUpgrade=true Test_5121_Upgrade "jdbc:derby:db" 3 false

          I have verified that Mamta's latest patch fixes the soft-upgrade problem for 1, 2, 3, and 4 column tables.

          Show
          Rick Hillegas added a comment - Attaching Test_5121_Upgrade.java. This is a modification of the previous test of column subsets and permutations. This splits the schema creation apart from the DML so that the schema can be created under 10.7 and the UPDATE statements and trigger-firings can be tested under 10.8. To run this test, do the following: 1) Create the schema by setting your classpath to include the 10.7 jars. Delete the db database. Then do the following to create the schema for 3 column tables: java Test_5121_Upgrade "jdbc:derby:db;create=true" 3 false 2) Now set your classpath to include the 10.8 jars. Test trigger-firing after soft-upgrade by doing the following: java -Dderby.database.allowPreReleaseUpgrade=true Test_5121_Upgrade "jdbc:derby:db" 3 false I have verified that Mamta's latest patch fixes the soft-upgrade problem for 1, 2, 3, and 4 column tables.
          Hide
          Mamta A. Satoor added a comment -

          Thanks Rick, I really appreciate all the testing you have done with my changes.

          The DERBY_5121_NotReadForCommit_patch4_trunk_diff.txt has been committed to trunk. I am now working on making the same engine changes to 10.7 codeline. The upgrade test can't be copied from trunk to 10,7 because at this point, there is no way to test upgrades between point releases. It will be good to have the framework in place. I will create a jira for that.

          Once the derbyall and junit run fine on 10.7, I will commit the changes on that codeline too.

          Show
          Mamta A. Satoor added a comment - Thanks Rick, I really appreciate all the testing you have done with my changes. The DERBY_5121_NotReadForCommit_patch4_trunk_diff.txt has been committed to trunk. I am now working on making the same engine changes to 10.7 codeline. The upgrade test can't be copied from trunk to 10,7 because at this point, there is no way to test upgrades between point releases. It will be good to have the framework in place. I will create a jira for that. Once the derbyall and junit run fine on 10.7, I will commit the changes on that codeline too.
          Hide
          Rick Hillegas added a comment -

          Attaching a second version of the detailed release note. This version gives users more information about what kinds of triggers are vulnerable to this corruption. I hope that I have described the situation accurately.

          Show
          Rick Hillegas added a comment - Attaching a second version of the detailed release note. This version gives users more information about what kinds of triggers are vulnerable to this corruption. I hope that I have described the situation accurately.
          Hide
          Mamta A. Satoor added a comment -

          Hi Rick, I looked at the release note and it looks good to me.

          Also, I have backported engine related changes from trunk(revision 1087049) to 10,7 codeline

          Show
          Mamta A. Satoor added a comment - Hi Rick, I looked at the release note and it looks good to me. Also, I have backported engine related changes from trunk(revision 1087049) to 10,7 codeline
          Hide
          Mamta A. Satoor added a comment -

          I do not forsee any further code changes related to backing out DERBY-1482 changes so I think it should be safe to resolve this issue so we don't have any blockers for 10.8 release.

          The right fix for DERBY-1482 can go in as part of DERBY-1482 even after 10.8 release is out. Those changes can then be made available as next point release of 10.8 and since at the time of upgrade, the trigger SPSes get marked invalid, any changes made for DERBY-1482 will get automatically picked up.

          Show
          Mamta A. Satoor added a comment - I do not forsee any further code changes related to backing out DERBY-1482 changes so I think it should be safe to resolve this issue so we don't have any blockers for 10.8 release. The right fix for DERBY-1482 can go in as part of DERBY-1482 even after 10.8 release is out. Those changes can then be made available as next point release of 10.8 and since at the time of upgrade, the trigger SPSes get marked invalid, any changes made for DERBY-1482 will get automatically picked up.
          Hide
          Mamta A. Satoor added a comment -

          I have converted Rick's test into junit test with revision 1125453. Rick. please feel free to add comments to the junit test if you think it needs any.

          Show
          Mamta A. Satoor added a comment - I have converted Rick's test into junit test with revision 1125453. Rick. please feel free to add comments to the junit test if you think it needs any.

            People

            • Assignee:
              Mamta A. Satoor
              Reporter:
              Rick Hillegas
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development