Update trigger updating the same rows as the original update does not throw an exception ERROR 54038: "Maximum depth of nested triggers was exceeded" as it should

• Key: DERBY-1652
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Yip Ng
• Reporter: Kathey Marsden
• Component/s: SQL
• Affects Version/s: 10.0.2.0, 10.1.3.1, 10.2.1.6
• Fix Version/s: 10.1.3.2, 10.2.1.6

Description

Execution of an update trigger that updates the same row as the original update will recurse forever and exceed the maximum nesting level of 16 so should throw the exception:
ERROR 54038: "Maximum depth of nested triggers was exceeded"

However, it does not always throw the exception. For example:


CREATE TABLE "TEST" (
      
 "TESTID" INTEGER NOT NULL GENERATED ALWAYS AS IDENTITY (START
 WITH 1,
 INCREMENT BY 1),
      
 "INFO" INTEGER NOT NULL,
      
 "TIMESTAMP" TIMESTAMP NOT NULL DEFAULT
 '1980-01-01-00.00.00.000000'
 );
      
 CREATE TRIGGER UPDATE_TEST
  AFTER UPDATE ON TEST
  REFERENCING OLD AS OLD
  FOR EACH ROW MODE DB2SQL
  UPDATE TEST SET TIMESTAMP = CURRENT_TIMESTAMP WHERE
  TESTID = OLD.TESTID;
 INSERT INTO TEST (INFO) VALUES
 (1),
 (2),
 (3);

 UPDATE TEST SET INFO = 1 WHERE TESTID = 2;

Does not throw an exception:

However, If the derby jars are updated to a new version, the correct exception is thrown.

 Replace derby jars with new version
 Execute the following in ij:
 UPDATE TEST SET INFO = 1 WHERE TESTID = 2;
 ERROR 54038: Maximum depth of nested triggers was exceeded.


Note: This issue stemmed from the Invalid issue, DERBY-1603, because a user hit the exception after upgrade and thought the exception after upgrade, not the lack of exception before upgrade was the problem. This may be a common user error, so we need a release note to help mitigate the issue. I will add one shortly after confirming the correct trigger syntax.

Comments

[Daniel John Debrunner]

Does the comment "However, If the derby jars are updated to a new version" mean that the bug is fixed in the new version and so thiis bug is already fixed.
Ie. it is just a problem with some older versions and upgrading to the latest 10.1 will mean the exception is always thrown, no matter how the trigger was created?

[Kathey Marsden]

Dan asked:
>Does the comment "However, If the derby jars are >updated to a new version" mean that the bug is fixed in >the new version and so thiis bug is already fixed.

No if you create this trigger in the latest 10.2 you will see that when it fires it does not give an exception.

It is changing versions that triggers the trigger to start giving the exception. So for instance if you create the trigger in 10.1.3.1, it would fire without error until you moved to 10.1.3.2. Then you would start seeing the exception when the trigger fires. This is why I am concerned about user impact with this issue. The incorrect trigger will operate as the user might think it would until they upgrade and then boom, their application works no more.

[Yip Ng]

Attaching patch for DERBY-1652. The patch is for the 10.1 codebase. A similar patch is needed for 10.2, which I'll post as soon as derbyall completes.

The cause of the problem is that the trigger descriptor is created after the stored prepared statement(SPS) has been compiled, so the compiled form of the SPS is not aware of that its trigger action can fire on the trigger table itself. Hence, the constant action was not generated correctly.

During upgrade, the SPSs are invalidated at database boot time. The SPS will be recompile when it is being invoked and the recompilation at this point will of course detect the relevent trigger and generate the correct constant action for the SPS and produce the expected behavior when the SPS is executed - throwing an error when it exceeds the trigger's maximum depth in the above case.

The simplest solution without introducing another revalidation of the SPS is to create the trigger descriptor first before compiling the SPS. I ran derbyall and had to go over the testcases which have the wrong master outputs and I have corrected them on this patch. Appreciate if someone can review this.

On a side note, I ran derbyall on a *clean* 10.1 codeline and I see the following
failures:

derbyall/derbyall.fail:unit/T_Diagnosticable.unit
derbyall/derbyall.fail:i18n/urlLocale.sql
derbyall/derbyall.fail:i18n/messageLocale.sql
derbyall/derbyall.fail:i18n/iepnegativetests_ES.sql
derbyall/derbynetclientmats/derbynetmats.fail:derbynet/sysinfo.java
derbyall/derbynetmats/derbynetmats.fail:derbynet/sysinfo.java

Is this a known problem?

[Yip Ng]

Attaching patch for DERBY-1652. This patch applies to the trunk codeline. Derbyall still running, will confirm if it passes successfully in subsequent comment. Nevertheless, the patch is ready for review.

[Yip Ng]

derbyall passes with derby1652-trunk-diff01.txt.

[Yip Ng]

This patch appears to add a duplicate entry to the dependency table. Need to investigate...

[Yip Ng]

False alarm. Renabling patch available.

[Dag H. Wanvik]

The patch look good.

When the trunk patch is applied I get the desired error on the repro,
without it, I do not. I also checked with soft and hard upgrade (from
10.1 trunk) and its still works in those cases. I did not try out the
10.1 version of the patch.

The diagnosis seems correct from looking locally at the code, although
I have not traced the logic fully to where the dependency actually
gets picked up.

Minor nit on the code: The variable tmpTriggerId may be done away
with, or at least its comment text updated; it is no longer so
meaningful now that the sps creation is moved out, but if you prefer,
I am ok with leaving that to the committer's discretion.

+1

[Yip Ng]

Thanks for the taking the time to review this patch, Dag. The local variable tmpTriggerId is kept because it is still needed later by the createSPS method. As for when the dependency will get picked up, it is at the trigger action's bind phase.

[Dag H. Wanvik]

Yes, I know it's used by the createSPS methods; my point was that you
could do away with it by using TriggerDecriptor#getUUID instead once
triggerd is created. In the old code, a variable was necessary, since
the trigger was referenced before creation. Just a detail, I agree.

[Kathey Marsden]

I don't know much about this code area but I looked at this patch as I am very keen to see this in to 10.2 and would also like to get it into 10.1 as soon as possible. The approach looks reasonable to me but I have to admit I don't understand the full impact of moving the trigger descriptor creation before compiling the SPS, so I don't have too much to offer in terms of code review.

On the tests, I have some questions about the the test updates
In updateableResultSet.java we have this diff.

                        rs.next();
                        System.out.println("column 1 on this row is " + rs.getInt(1));
                        System.out.println("this update row will fire the update trigger which will update all the rows in the table to have c1=1 and hence no more rows will qualify for the resultset"
);
- rs.updateLong(1,123);
+ rs.updateLong(2,2);
                        rs.updateRow();
                        rs.next();

By changing a different value are we still performing the action described?

Similarly in triggerGeneral master update we had previously positive test cases that now throw an error. Might we be able to use the workaround you gave me, to specify on which columns the trigger will fire to retain the original semantics of these tests?

[Yip Ng]

For the updatableResultSet.java, yes, it will still perform the action described because the triggers are also modified to preserve the original intention of the test.

The master output for triggerGeneral.sql were incorrect for those testcases, so I just replaced them with the expected results (I thought they were "negative" tests with wrong results). I tried to preserve the original testcase intent whenever I can but the column list specification won't work on those testcases since they are using an "after insert" trigger. And that test file already have tests to insert to another table from the trigger action, so now those tests really become "negative"...

[Mike Matrigali]

I am looking at committing the trunk submitted diff, assuming there is no objection. It seems like Dag's comment can be handled as a separate item. To this end I ran all the tests on XP, sun jdk1.4.2 and got
3 diffs:
derbyall/derbynetmats/DerbyNet/jdbcapi/blobclob4BLOB.diff (known issue showing up in the tinderbox tests)
derbyall/upgrade/Upgrade_10_1_10_2.diff (known issue showing up in the tinderbox tests)
and the following I need to investigate:
*** Start: _Suite jdk1.4.2_07 DerbyNetClient derbynetmats:jdbcapi 2006-08-14 19:
57:40 ***
0 add
> ..................................E...E.
> There were 2 errors:
> 1) testUpdateUpdatedTupleScrollUpdateRow(org.apache.derbyTesting.functionTests
.tests.jdbcapi.URCoveringIndexTest)java.lang.OutOfMemoryError
> 2) testNextOnLastRowForwardOnly(org.apache.derbyTesting.functionTests.tests.jd
bcapi.ScrollResultSetTest)java.lang.OutOfMemoryError
> FAILURES!!!
> Tests run: 817, Failures: 0, Errors: 2
Test Failed.
*** End: _Suite jdk1.4.2_07 DerbyNetClient derbynetmats:jdbcapi 2006-08-14 20:
01:56 ***

[Kathey Marsden]

Yip said
>I thought they were "negative" tests with wrong results
Thanks Yip, I think I got confused because of the comments: "--after" and the trigger name: "tgood" don't lend themselves to a negative test. If more work is done on the patch it might be good to change them for clarity moving forward.

ij> -- after
create trigger tgood after insert on x for each statement mode db2sql insert into x values 666;
0 rows inserted/updated/deleted
ij> insert into x values 1;
ERROR 54038: Maximum depth of nested triggers was exceeded.

[Mike Matrigali]

I reproduced the out of memory on my machine in a clean codeline so do not believe it is caused by this
patch. I also have reported DERBY-1701 for those out of memory errors which have been showing up in the public regression test runs on a number of platforms.

I committed this patch as is, feeling the comments above were not worth holding up the patch. They seem like good things to do, but not necessary to hold up the fix.

m1_142:97>svn commit

Sending java\engine\org\apache\derby\impl\sql\execute\CreateTriggerConstantAction.java
Sending java\engine\org\apache\derby\impl\store\raw\data\OverflowInputStream.java
Sending java\testing\org\apache\derbyTesting\functionTests\master\triggerGeneral.out
Sending java\testing\org\apache\derbyTesting\functionTests\tests\lang\triggerGeneral.sql
Sending java\testing\org\apache\derbyTesting\functionTests\tests\lang\updatableResultSet.java
Transmitting file data .....
Committed revision 431698.

[Yip Ng]

I think this issue should be considered as resolved once the changes have also been updated to 10.1 codeline.

[Kathey Marsden]

Thanks Yip. Committed to 10.1

Date: Wed Aug 16 23:32:27 2006
New Revision: 432165

URL: http://svn.apache.org/viewvc?rev=432165&view=rev
We still need the release note as users may hit the error unexpectedly on upgrade and may need to change their triggers to specify which column in the trigger that will invoke the trigger action per Yip's suggestion at:
http://www.nabble.com/Re%3A--jira--Commented%3A-%28DERBY-1603%29-ERROR-54038%3A-%22Maximum-depth-of-nested-triggers-was-exceeded%22-occurs-when-trigger-fires-after-upating-10.1.2.5-jars-to-10.1.3.1-p5689247.html

I'm afraid I can't get to that right away after all. It will have to wait a while if I do it.

Thanks

Kathey

[Yip Ng]

Here is the release note for this jira:


Release Note for DERBY-1652
-------------------------------------------

PROBLEM

   In some cases, an after update trigger does not get fired upon itself when its trigger action contains
an update statement on the trigger's subject table.


SYMPTOMS

   (1) When defining a trigger for the first time for a table, e.g.:

  
        CREATE TABLE "TEST" ("TESTID" INTEGER NOT NULL GENERATED ALWAYS AS IDENTITY (START WITH 1, INCREMENT BY 1),
                             "INFO" INTEGER NOT NULL,
                             "TIMESTAMP" TIMESTAMP NOT NULL DEFAULT '1980-01-01-00.00.00.000000');
    
        CREATE TRIGGER UPDATE_TEST
        AFTER UPDATE ON TEST
        REFERENCING OLD AS OLD
        FOR EACH ROW MODE DB2SQL
            UPDATE TEST SET TIMESTAMP = CURRENT_TIMESTAMP WHERE TESTID = OLD.TESTID;
  
        INSERT INTO TEST (INFO) VALUES (1), (2), (3);

        UPDATE TEST SET INFO = 1 WHERE TESTID = 2;


        The above update statement executes successfully which it is incorrect. The system should have issued
        SQLSTATE 54038 since it self-triggers to its maximum depth of 16.

   
   (2) With the above example, when an user upgrades to a higher version and issues the same update statement:

        UPDATE TEST SET INFO = 1 WHERE TESTID = 2;
        ERROR 54038: Maximum depth of nested triggers was exceeded.

The SQLSTATE 54038 is issued in this case because after database upgrade, the trigger action will be
        invalidated by the system and will force a recompilation of the trigger when it is fired. The system
        generates the correct execution plan this time and since the trigger behavior have changed, this might
        cause applications to break unexpectedly.


CAUSE

   Derby's did not generate the correct execution plan for self-trigger invocation when such a trigger is declared
   for the first time on the subject table; hence, resulting in the stated problem above. The affected version is
   Derby 10.0 and 10.1.


SOLUTION

   A fix to resolve the above Derby symptom is available in 10.1 and 10.2.


WORKAROUND

   If self-trigger invocation was not intended by the application, the application can select which column(s) on the
   update statement can cause the trigger to fire in the cREATE TRIGGER statement. i.e.:

   CREATE TRIGGER update_test
   AFTER UPDATE OF INFO ON test
   REFERENCING OLD AS old
   FOR EACH ROW MODE DB2SQL
       UPDATE test SET timestamp=current_timestamp WHERE testid=old.testid;
 
   In the above statement, the trigger will only fire when an update is made to the "info" column instead of any column(s).