Derby
  1. Derby
  2. DERBY-5044

ALTER TABLE DROP COLUMN will not detect triggers defined on other tables with their trigger action using the column being dropped

    Details

    • Urgency:
      Normal
    • Bug behavior facts:
      Deviation from standard

      Description

      A trigger in it's trigger action.can use columns from a table other than the trigger table. When such a column is dropped, the trigger dependency does not get detected.

      A test case for this can be found in AlterTableTest.java
      //Following test case involves two tables. The trigger is defined
      //on table 1 and it uses the column from table 2 in it's trigger
      //action. This dependency of the trigger on a column from another
      //table is not detected by Derby.
      st.executeUpdate("create table atdc_14_tab1 (a1 integer, b1 integer)");
      st.executeUpdate("create table atdc_14_tab2 (a2 integer, b2 integer)");
      st.executeUpdate("insert into atdc_14_tab1 values(1,11)");
      st.executeUpdate("insert into atdc_14_tab2 values(1,11)");
      st.executeUpdate(
      " create trigger atdc_14_trigger_1 after update " +
      "on atdc_14_tab1 REFERENCING NEW AS newt " +
      "for each row " +
      "update atdc_14_tab2 set a2 = newt.a1");

      // following is not the right behavior. we should have gotten an error
      // because column being dropped is getting used in a trigger action
      st.executeUpdate("alter table atdc_14_tab2 drop column a2 restrict");
      rs =
      st.executeQuery(
      " select triggername from sys.systriggers where " +
      "triggername = 'ATDC_14_TRIGGER_1' ");
      JDBC.assertFullResultSet(rs, new String[][]"ATDC_14_TRIGGER_1");

      1. DERBY_5044_backportTo108_diff_patch1.txt
        86 kB
        Mamta A. Satoor
      2. DERBY_5044_diff_patch1.txt
        21 kB
        Mamta A. Satoor

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          [bulk update] Close all resolved issues that haven't been updated for more than one year.

          Show
          Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.
          Hide
          Mamta A. Satoor added a comment -

          If we do not have a customer request for backporting DERBY-5044, then I would rather us not backport this jira. The primary reason is we did work on several trigger related jiras around the same time and some of those jiras depended on changes to system tables. Those different jiras worked on quite a few common classes and around the same code and hence I am worried that we might backport something which depends on codes which should not be backported.

          I spent some time over this weekend trying to backport DERBY-5044 and found that I had to hand tweak quite a few merges for just one revision. DERBY-5044 has more than one revisions to checkin all the needed changes.

          Show
          Mamta A. Satoor added a comment - If we do not have a customer request for backporting DERBY-5044 , then I would rather us not backport this jira. The primary reason is we did work on several trigger related jiras around the same time and some of those jiras depended on changes to system tables. Those different jiras worked on quite a few common classes and around the same code and hence I am worried that we might backport something which depends on codes which should not be backported. I spent some time over this weekend trying to backport DERBY-5044 and found that I had to hand tweak quite a few merges for just one revision. DERBY-5044 has more than one revisions to checkin all the needed changes.
          Hide
          Kathey Marsden added a comment -

          Reopen for 10.5 backport consideration. If working on the backport for this issue. Temporarily assign yourself and add a comment that you are working on it. When finished, reresolve with the new fix versions or label backport_reject_10_x as appropriate.

          Show
          Kathey Marsden added a comment - Reopen for 10.5 backport consideration. If working on the backport for this issue. Temporarily assign yourself and add a comment that you are working on it. When finished, reresolve with the new fix versions or label backport_reject_10_x as appropriate.
          Hide
          Mamta A. Satoor added a comment -

          Backported to 10.8

          Show
          Mamta A. Satoor added a comment - Backported to 10.8
          Hide
          Mamta A. Satoor added a comment -

          Attaching patch DERBY_5044_backportTo108_diff_patch1.txt which will allow us to backport the changes for this jira to 10.8 release. The derbyall suite ran fine with no errors. Junit suite had known failures including testCS4595B_UniqueIndex failure which is DERBY-4540(I added a comment to that jira). This backport might be too late for the current release of 10.8.2.1 but I wanted to attach the patch anyways so we know that it is ready for backport.

          Show
          Mamta A. Satoor added a comment - Attaching patch DERBY_5044_backportTo108_diff_patch1.txt which will allow us to backport the changes for this jira to 10.8 release. The derbyall suite ran fine with no errors. Junit suite had known failures including testCS4595B_UniqueIndex failure which is DERBY-4540 (I added a comment to that jira). This backport might be too late for the current release of 10.8.2.1 but I wanted to attach the patch anyways so we know that it is ready for backport.
          Hide
          Myrna van Lunteren added a comment -

          Mamta committed the patch with revision: http://svn.apache.org/viewvc?view=revision&revision=1166313
          (adding a comment because initially the JIRA number was skipped and JIRA doesn't pick up commits that have been modified with svn propedit).

          Show
          Myrna van Lunteren added a comment - Mamta committed the patch with revision: http://svn.apache.org/viewvc?view=revision&revision=1166313 (adding a comment because initially the JIRA number was skipped and JIRA doesn't pick up commits that have been modified with svn propedit).
          Hide
          Mamta A. Satoor added a comment -

          I am attaching a patch(DERBY5044_diff_patch1.txt) for review. This patch implements the logic explained on August 9th 2011. To reiterate,for the table being altered, we will go through the dependency system to determine all the triggers that depend on the table being altered(this will include triggers defined directly on the table being altered and the triggers defined on other tables but using the table being altered in their trigger action plan.) This is done by first finding all the objects that depend on the table being altered. We are only interested in SPSDescriptors from that list of dependent objects. For each of these dependent SPSDescriptor, we want to find if they are defined for a trigger action SPS. If yes, then the trigger must be dependent on the table being altered. For each of these dependent triggers, we dropped their trigger descriptor from datadictionary, regenerate and rebind it's trigger action SPS and then add the trigger descriptor(with upto date version of internal representation of trigger action) back to datadictionary.During the rebind of trigger action, we will get exception if the trigger depends on the column being altered. If so, then if the alter table drop column is being done in restrict mode, then we will throw an exception that column can't be dropped because it has dependent object. If the drop column was issued in cascade mode, then we will drop the dependent triggers.

          Please let me know if you have any comments on the attached patch. I do want to point out that in this patch, I have removed the code which used to go directly through all the triggers defined on the table being altered and dropping, rebinding and recreating them. This is because the new code going through the dependency system should find all the triggers which would be impacted by drop column, no matter whether the triggers are defined on the table being altered or triggers defined on other tables but using table being altered in their trigger action.) DERBY-5120 could have prevented us from catching all the triggers defined on the table being altered through the dependency system because of missing dependency between trigger action sps and trigger table but that has been fixed in 10.9 and 10.8 so we should be fine. I have run all the existing junit suites and derbyall with this patch and they ran fine.

          I will work next on writing test cases.

          Show
          Mamta A. Satoor added a comment - I am attaching a patch(DERBY5044_diff_patch1.txt) for review. This patch implements the logic explained on August 9th 2011. To reiterate,for the table being altered, we will go through the dependency system to determine all the triggers that depend on the table being altered(this will include triggers defined directly on the table being altered and the triggers defined on other tables but using the table being altered in their trigger action plan.) This is done by first finding all the objects that depend on the table being altered. We are only interested in SPSDescriptors from that list of dependent objects. For each of these dependent SPSDescriptor, we want to find if they are defined for a trigger action SPS. If yes, then the trigger must be dependent on the table being altered. For each of these dependent triggers, we dropped their trigger descriptor from datadictionary, regenerate and rebind it's trigger action SPS and then add the trigger descriptor(with upto date version of internal representation of trigger action) back to datadictionary.During the rebind of trigger action, we will get exception if the trigger depends on the column being altered. If so, then if the alter table drop column is being done in restrict mode, then we will throw an exception that column can't be dropped because it has dependent object. If the drop column was issued in cascade mode, then we will drop the dependent triggers. Please let me know if you have any comments on the attached patch. I do want to point out that in this patch, I have removed the code which used to go directly through all the triggers defined on the table being altered and dropping, rebinding and recreating them. This is because the new code going through the dependency system should find all the triggers which would be impacted by drop column, no matter whether the triggers are defined on the table being altered or triggers defined on other tables but using table being altered in their trigger action.) DERBY-5120 could have prevented us from catching all the triggers defined on the table being altered through the dependency system because of missing dependency between trigger action sps and trigger table but that has been fixed in 10.9 and 10.8 so we should be fine. I have run all the existing junit suites and derbyall with this patch and they ran fine. I will work next on writing test cases.
          Hide
          Mamta A. Satoor added a comment -

          Updated list of tests that I plan to add for this jira
          1)Trigger actions with INSERT sql using column being dropped(from a non-trigger table)
          2)Trigger actions with UPDATE sql using column being dropped(from a non-trigger table)
          3)Trigger actions with SELECT from VIEWS with views and trigger action using column being dropped(from a non-trigger table)
          4)Have a combination of trigger types in the same test ie trigger defined on the table being altered and trigger defined on other tables but using the table being altered in their trigger action
          5)We have test for VIEWS, but see if we need to be testing for SYNONYMS too
          6)In upgrade, test triggers who have lost one of their dependency row(DERBY-5120) and see how they work when they are using the table who column is being dropped
          7)In upgrade, test triggers using column being altered and show how it is not detected prior to 10.9
          8)Trigger action with SELECT from table using column being dropped from non-trigger table(we have the VIEWs variation of that test already)
          9)Add test to check how privileges might affect the trigger recompile by a user who doesn't own the trigger

          Show
          Mamta A. Satoor added a comment - Updated list of tests that I plan to add for this jira 1)Trigger actions with INSERT sql using column being dropped(from a non-trigger table) 2)Trigger actions with UPDATE sql using column being dropped(from a non-trigger table) 3)Trigger actions with SELECT from VIEWS with views and trigger action using column being dropped(from a non-trigger table) 4)Have a combination of trigger types in the same test ie trigger defined on the table being altered and trigger defined on other tables but using the table being altered in their trigger action 5)We have test for VIEWS, but see if we need to be testing for SYNONYMS too 6)In upgrade, test triggers who have lost one of their dependency row( DERBY-5120 ) and see how they work when they are using the table who column is being dropped 7)In upgrade, test triggers using column being altered and show how it is not detected prior to 10.9 8)Trigger action with SELECT from table using column being dropped from non-trigger table(we have the VIEWs variation of that test already) 9)Add test to check how privileges might affect the trigger recompile by a user who doesn't own the trigger
          Hide
          Dag H. Wanvik added a comment -

          That's good news, Mamta! Thanks!

          Show
          Dag H. Wanvik added a comment - That's good news, Mamta! Thanks!
          Hide
          Mamta A. Satoor added a comment -

          Hi Dag, I looked through the code for trigger recompilation and found that we only drops the dependencies related to the trigger action's stored prepared statement and dependency between the trigger and trigger action sps. The trigger's dependency on various privileges do not get dropped. This is because these privilege dependencies get dropped when drop() method is called on the TriggerDescriptor. As part of the trigger recompilation, we do not call TriggerDescriptor.drop. Instead, we call DataDictionary.dropTriggerDescriptor(trd, tc), we recompile the trigger action sps and then call DataDictionary.addDescriptor(trd, sd, DataDictionary.SYSTRIGGERS_CATALOG_NUM, false, tc). So, even though the trigger gets recompiled as internal trigger, the trigger's dependency on various privileges do not get lost. Those privileges were added as part of the original create trigger sql in CreateTriggerConstant.storeViewTriggerDependenciesOnPrivileges and do not get touched during the recompile process. Hope this answers your question.

          I will make sure that I write tests showing that privilege requirement didn't get dropped.

          Show
          Mamta A. Satoor added a comment - Hi Dag, I looked through the code for trigger recompilation and found that we only drops the dependencies related to the trigger action's stored prepared statement and dependency between the trigger and trigger action sps. The trigger's dependency on various privileges do not get dropped. This is because these privilege dependencies get dropped when drop() method is called on the TriggerDescriptor. As part of the trigger recompilation, we do not call TriggerDescriptor.drop. Instead, we call DataDictionary.dropTriggerDescriptor(trd, tc), we recompile the trigger action sps and then call DataDictionary.addDescriptor(trd, sd, DataDictionary.SYSTRIGGERS_CATALOG_NUM, false, tc). So, even though the trigger gets recompiled as internal trigger, the trigger's dependency on various privileges do not get lost. Those privileges were added as part of the original create trigger sql in CreateTriggerConstant.storeViewTriggerDependenciesOnPrivileges and do not get touched during the recompile process. Hope this answers your question. I will make sure that I write tests showing that privilege requirement didn't get dropped.
          Hide
          Dag H. Wanvik added a comment -

          Hi Mamta, again I am thinking of the compilation context: if the original compilation context had a role set and that role was necessary to be able to compile the trigger, a dependency is registered for that trigger on the role, too (cf RolesConferredPrivilegesTest#testTriggerInvalidation). If you do an internal compile, I presume that dependency would be lost. One approach could be to take note of the possible role dependency and add it back when the trigger is recreated.

          Show
          Dag H. Wanvik added a comment - Hi Mamta, again I am thinking of the compilation context: if the original compilation context had a role set and that role was necessary to be able to compile the trigger, a dependency is registered for that trigger on the role, too (cf RolesConferredPrivilegesTest#testTriggerInvalidation). If you do an internal compile, I presume that dependency would be lost. One approach could be to take note of the possible role dependency and add it back when the trigger is recreated.
          Hide
          Mamta A. Satoor added a comment -

          I found that given a table descriptor, we can use DataDictionary.getProvidersDescriptorList to get the list of all the objects that depend on the table descriptor. Once we have all those objects, we can look through them to find just the stored prepared statements since our ultimate goal is to find all the triggers who might be using the column being dropped in their trigger action plans. For every dependent stored prepared statement, we will check it's dependents (same as we did for table descriptor using DataDictionary.getProvidersDescriptorList). If the dependent on the stored prepared statement is a trigger then we know that the trigger action plan is using the table which is getting altered. We will pick these triggers, drop and recreate them after regenerating their trigger action plan and rebinding that plan. If we run into error during this process, then we know that we have found a depedent trigger and hence alter table drop column restrict will fail and cascade will drop the dependent trigger. I will look into coding this and post a patch soon. Please let me know if anyone has feedback on this approach. Thanks

          Show
          Mamta A. Satoor added a comment - I found that given a table descriptor, we can use DataDictionary.getProvidersDescriptorList to get the list of all the objects that depend on the table descriptor. Once we have all those objects, we can look through them to find just the stored prepared statements since our ultimate goal is to find all the triggers who might be using the column being dropped in their trigger action plans. For every dependent stored prepared statement, we will check it's dependents (same as we did for table descriptor using DataDictionary.getProvidersDescriptorList). If the dependent on the stored prepared statement is a trigger then we know that the trigger action plan is using the table which is getting altered. We will pick these triggers, drop and recreate them after regenerating their trigger action plan and rebinding that plan. If we run into error during this process, then we know that we have found a depedent trigger and hence alter table drop column restrict will fail and cascade will drop the dependent trigger. I will look into coding this and post a patch soon. Please let me know if anyone has feedback on this approach. Thanks
          Hide
          Mamta A. Satoor added a comment -

          Dag, thanks for your comment. I was on vacation so couldn't reply sooner. I will look into what happens if the trigger is created by a different user than the one performing the alter table. Maybe, it can compiled as internal sql which may be doesn't check the ownership.

          Show
          Mamta A. Satoor added a comment - Dag, thanks for your comment. I was on vacation so couldn't reply sooner. I will look into what happens if the trigger is created by a different user than the one performing the alter table. Maybe, it can compiled as internal sql which may be doesn't check the ownership.
          Hide
          Dag H. Wanvik added a comment -

          Referring to https://issues.apache.org/jira/browse/DERBY-5044?focusedCommentId=13068799&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13068799, I think that approach looks promising. Do we always have the necessary compilation context to regenerate the trigger actions if the triggers were defined by another user than the alter table user? In theory that context might include a current role for the trigger definer, too... ?

          Show
          Dag H. Wanvik added a comment - Referring to https://issues.apache.org/jira/browse/DERBY-5044?focusedCommentId=13068799&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13068799 , I think that approach looks promising. Do we always have the necessary compilation context to regenerate the trigger actions if the triggers were defined by another user than the alter table user? In theory that context might include a current role for the trigger definer, too... ?
          Hide
          Mamta A. Satoor added a comment -

          I was also thinking about what kind of tests can be added for this jira.

          I found that we have already added following tests where we show how the test allows wrong behavior(these tests are in AlterTableTest.java)
          1)Trigger actions with INSERT sql using column being dropped(from a non-trigger table)
          2)Trigger actions with UPDATE sql using column being dropped(from a non-trigger table)
          3)Trigger actions with SELECT from VIEWS with views and trigger action using column being dropped(from a non-trigger table)

          Following are some additional tests that we can add
          1)Have a combination of trigger types in the same test ie trigger defined on the table being altered and trigger defined on other tables but using the table being altered in their trigger action
          2)We have test for VIEWS, but see if we need to be testing for SYNONYMS too
          3)In upgrade, test triggers who have lost one of their dependency row(DERBY-5120) and see how they work when they are using the table who column is being dropped
          4)In upgrade, test triggers using column being altered and show how it is not detected prior to 10.9
          5)Trigger action with SELECT from table using column being dropped from non-trigger table(we have the VIEWs variation of that test already)

          I will appreciate if someone has any ideas on any more possible tests that should be written for this jira.

          Show
          Mamta A. Satoor added a comment - I was also thinking about what kind of tests can be added for this jira. I found that we have already added following tests where we show how the test allows wrong behavior(these tests are in AlterTableTest.java) 1)Trigger actions with INSERT sql using column being dropped(from a non-trigger table) 2)Trigger actions with UPDATE sql using column being dropped(from a non-trigger table) 3)Trigger actions with SELECT from VIEWS with views and trigger action using column being dropped(from a non-trigger table) Following are some additional tests that we can add 1)Have a combination of trigger types in the same test ie trigger defined on the table being altered and trigger defined on other tables but using the table being altered in their trigger action 2)We have test for VIEWS, but see if we need to be testing for SYNONYMS too 3)In upgrade, test triggers who have lost one of their dependency row( DERBY-5120 ) and see how they work when they are using the table who column is being dropped 4)In upgrade, test triggers using column being altered and show how it is not detected prior to 10.9 5)Trigger action with SELECT from table using column being dropped from non-trigger table(we have the VIEWs variation of that test already) I will appreciate if someone has any ideas on any more possible tests that should be written for this jira.
          Hide
          Mamta A. Satoor added a comment -

          I spent some time to see how we can find out all the triggers that might be using a given table in their trigger actions. This way, when a column is getting dropped from a table, we can detect dependent triggers. If the drop column is to be run in cascade mode, them the dependent triggers will be dropped. If the drpo column is to be run in restrict mode, then the drop will fail if any dependent triggers are found,

          What I found is that there is no direct way to find such triggers but we can get to them through a little round about way. All the stored prepared statements (including the ones created for trigger actions) record all their dependencies in SYSDEPENDS. So, if a stored prepared statement is using 2 tables in trigger action, both those dependencies will be put in SYSDEPENDS. We can build our logic based on this information.

          Anytime a column is getting dropped, we will look at SYSDEPENDS and find all the stored prepared statement that are using the table whose column is getting dropped. Then we will look at those stored prepared statements in SYSDEPENDS to see if they depend on a trigger descriptor. If yes, then we know that this stored prepared statement is for a trigger's trigger action. With this logic, we can find all the triggers which might be using a given table in their trigger action. Now, if the ALTER TABLE DROP COLUMN is getting run, then we will drop all these dependent triggers, regenerate their trigger action and then recreate the triggers. During the regeneration fo trigger action, if the trigger action is using the column being dropped, the regeneration will fail and we will know that this trigger is using the column being dropped. With this information, if we are running ALTER TABLE DROP COLUMN with restrict, then ALTER TABLE will fail because we found a trigger using the column being dropped. If we are running ALTER TABLE DROP COLUMN with cascade, then we will simply drop the trigger and not recreate it.

          Please let me know if anyone has any feedback on this approach. Thanks

          Show
          Mamta A. Satoor added a comment - I spent some time to see how we can find out all the triggers that might be using a given table in their trigger actions. This way, when a column is getting dropped from a table, we can detect dependent triggers. If the drop column is to be run in cascade mode, them the dependent triggers will be dropped. If the drpo column is to be run in restrict mode, then the drop will fail if any dependent triggers are found, What I found is that there is no direct way to find such triggers but we can get to them through a little round about way. All the stored prepared statements (including the ones created for trigger actions) record all their dependencies in SYSDEPENDS. So, if a stored prepared statement is using 2 tables in trigger action, both those dependencies will be put in SYSDEPENDS. We can build our logic based on this information. Anytime a column is getting dropped, we will look at SYSDEPENDS and find all the stored prepared statement that are using the table whose column is getting dropped. Then we will look at those stored prepared statements in SYSDEPENDS to see if they depend on a trigger descriptor. If yes, then we know that this stored prepared statement is for a trigger's trigger action. With this logic, we can find all the triggers which might be using a given table in their trigger action. Now, if the ALTER TABLE DROP COLUMN is getting run, then we will drop all these dependent triggers, regenerate their trigger action and then recreate the triggers. During the regeneration fo trigger action, if the trigger action is using the column being dropped, the regeneration will fail and we will know that this trigger is using the column being dropped. With this information, if we are running ALTER TABLE DROP COLUMN with restrict, then ALTER TABLE will fail because we found a trigger using the column being dropped. If we are running ALTER TABLE DROP COLUMN with cascade, then we will simply drop the trigger and not recreate it. Please let me know if anyone has any feedback on this approach. Thanks
          Hide
          Mamta A. Satoor added a comment -

          I am investigating this jira to see if there is enough information in SYSDEPENDS that we can use to determine the triggers impacted by an ALTER TABLE drop column.

          Show
          Mamta A. Satoor added a comment - I am investigating this jira to see if there is enough information in SYSDEPENDS that we can use to determine the triggers impacted by an ALTER TABLE drop column.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development