Derby
  1. Derby
  2. DERBY-4988

ALTER TABLE DROP COLUMN should make use of information in SYSTRIGGERS to detect column used through REFERENCING clause to find trigger dependencies

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.7.1.1
    • Fix Version/s: 10.7.1.4, 10.8.1.2
    • Component/s: SQL
    • Labels:
      None

      Description

      At the time of ALTER TABLE DROP COLUMN, Derby checks if the column being dropped in a trigger column and if so, then it will not drop the column if it is being done in RESTRICT mode or it will drop the trigger while dropping the column in CASCADE mode. This does not implement SQL standard to it's entirety.

      **************************************
      SQL standard says following about ALTER TABLE DROP COLUMN RESTRICT and trigger dependency in CREATE TRIGGER section

      If RESTRICT is specified, then C shall not be referenced in any of the following
      d) Either an explicit trigger column list or a triggered action column set of any trigger descriptor.
      (The triggered action column set included in the trigger descriptor is the set of all distinct, fully qualified names of columns contained in the <triggered action>.)
      **************************************

      What is missing from Derby implementation from SQL standard point of view is detected triggered action column set.

      Starting 10.7(with DERBY-1482), Derby started keeping track of trigger action columns which are referenced through REFERENCING clause. This information can be used to improve the behavior of ALTER TABLE DROP COLUMN in 10.7 and higher. This will not cover all the trigger action columns since columns referenced without the REFERENCING clause are not tracked anywhere at this point. More work will need to be done to implement SQL standard completely. But we can take a step forward by using the information available in 10.7 and higher to detect trigger action columns which are referenced through REFERENCING clause

        Issue Links

          Activity

          Hide
          Dag H. Wanvik added a comment -

          Thanks for the patch, Mamta! I am going to read through it as as soon as I find the time.
          It's good that you have added so many comments throughout, that is a great help. Sometimes, I find them a bit dense to read, though: perhaps it would improve legibility to always add a space between the comment start "//" and the text. And throw in some blank lines where natural, too, maybe.

          Show
          Dag H. Wanvik added a comment - Thanks for the patch, Mamta! I am going to read through it as as soon as I find the time. It's good that you have added so many comments throughout, that is a great help. Sometimes, I find them a bit dense to read, though: perhaps it would improve legibility to always add a space between the comment start "//" and the text. And throw in some blank lines where natural, too, maybe.
          Hide
          Mamta A. Satoor added a comment -

          Committed the changes to trunk with following commit comments

          DERBY-4988 ALTER TABLE DROP COLUMN should make use of information in SYSTRIGGERS to detect column used through REFERENCING clause to find trigger dependencies

          Derby at the time of ALTER TABLE DROP COLUMN looks for trigger dependencies by looking for column being dropped in trigger column list but that is not enough. SQL standard requires that column should not be part of explicit trigger column list or a triggered action column set.

          starting Derby 10.7, we have started keeping track of trigger action columns which are referenced through the REFERENCING clause. This commit will make use of that additional info to make a step forward towards meeting the SQL standards. It still does not recognize the trigger action columns that are not part of REFERENCING clause. That work can go separately.

          I have added upgrade test to make sure that the compatibility does not break between Derby releases prior to 10.7 and forward.

          Show
          Mamta A. Satoor added a comment - Committed the changes to trunk with following commit comments DERBY-4988 ALTER TABLE DROP COLUMN should make use of information in SYSTRIGGERS to detect column used through REFERENCING clause to find trigger dependencies Derby at the time of ALTER TABLE DROP COLUMN looks for trigger dependencies by looking for column being dropped in trigger column list but that is not enough. SQL standard requires that column should not be part of explicit trigger column list or a triggered action column set. starting Derby 10.7, we have started keeping track of trigger action columns which are referenced through the REFERENCING clause. This commit will make use of that additional info to make a step forward towards meeting the SQL standards. It still does not recognize the trigger action columns that are not part of REFERENCING clause. That work can go separately. I have added upgrade test to make sure that the compatibility does not break between Derby releases prior to 10.7 and forward.
          Hide
          Mamta A. Satoor added a comment -

          Thanks for taking the time to review the changes, Dag. I will keep your tip in mind about comments.

          Show
          Mamta A. Satoor added a comment - Thanks for taking the time to review the changes, Dag. I will keep your tip in mind about comments.
          Hide
          Mamta A. Satoor added a comment -

          I will look at backporting this jira to 10.7 once the tests have run successfully on IBM and Sun machines for few nights. They ran fine on my windows XP machine with IBM 16 jdk.

          Also, the changes for this jira should not be backported further back than 10.7. Prior to 10.7, we didn't keep track of trigger action column used through REFERENCING clause and the changes for this jira relies on that information,

          Show
          Mamta A. Satoor added a comment - I will look at backporting this jira to 10.7 once the tests have run successfully on IBM and Sun machines for few nights. They ran fine on my windows XP machine with IBM 16 jdk. Also, the changes for this jira should not be backported further back than 10.7. Prior to 10.7, we didn't keep track of trigger action column used through REFERENCING clause and the changes for this jira relies on that information,
          Hide
          Dag H. Wanvik added a comment -

          Some comment to the patch:

          • AlterTableConstantAction
          • // otherwsie there would be unexpected behaviors

          Apart from the typo, is this not SQL requirement?

          • The "change" variable can probably be eliminated, since this test is
            repeated over again at end of loop:

          referencedCols[j] > droppedColumnPosition

          i.e. the first test inside the loop to set "change" could be
          omitted, too. Test test on "j == refColLen" is a bit opaque, as far
          as I can tell it means here that we did see the trigger does not
          depend on the column being dropped, but may need adjustment of one
          or more of its columns. May be worth a clarification. Could the
          column index adjustment be moved outside the loop, perhaps (we are
          logically done when j == refColLen).

          • changedColPositionInTriggerAction is set but not used anywhere?
            Shouldn't the positions in referencedColsInTriggerAction be adjusted
            above droppedColumnPosition, too?
          • AlterTableTest
          • In the last test case added:
            //Now try ALTER TABLE DROP COLUMN CASCADE where the column being
            //dropped is in trigger action of trigger defined on a different table

          the test then goes on to say:
          // following is not the right behavior. we should have gotten an error
          // because column being dropped is getting used in a trigger action

          I thought that the issue here would be that the trigger isn't
          dropped? (since we have cascade mode)

          Show
          Dag H. Wanvik added a comment - Some comment to the patch: AlterTableConstantAction // otherwsie there would be unexpected behaviors Apart from the typo, is this not SQL requirement? The "change" variable can probably be eliminated, since this test is repeated over again at end of loop: referencedCols [j] > droppedColumnPosition i.e. the first test inside the loop to set "change" could be omitted, too. Test test on "j == refColLen" is a bit opaque, as far as I can tell it means here that we did see the trigger does not depend on the column being dropped, but may need adjustment of one or more of its columns. May be worth a clarification. Could the column index adjustment be moved outside the loop, perhaps (we are logically done when j == refColLen). changedColPositionInTriggerAction is set but not used anywhere? Shouldn't the positions in referencedColsInTriggerAction be adjusted above droppedColumnPosition, too? AlterTableTest In the last test case added: //Now try ALTER TABLE DROP COLUMN CASCADE where the column being //dropped is in trigger action of trigger defined on a different table the test then goes on to say: // following is not the right behavior. we should have gotten an error // because column being dropped is getting used in a trigger action I thought that the issue here would be that the trigger isn't dropped? (since we have cascade mode)
          Hide
          Mamta A. Satoor added a comment -

          Dag, thanks for taking the time to review the patch. You comments/questions were as follows
          Comments on AlterTableConstantAction
          1)You are right about following. This was a pre-existing comment. I will get rid of it because what we are doing follows the SQL standards.

          • // otherwsie there would be unexpected behaviors
            Apart from the typo, is this not SQL requirement?

          2)
          The "change" variable can probably be eliminated, since this test is repeated over again at end of loop:

          The code about the "change" variable is pre-existing too. Following is why I think it may have been coded this way.
          Inside the "if (j == refColLen && changed)" true code, we drop the trigger and then adjust the column positions of the affected trigger columns and then recreate the trigger with correct column positions. We drop the trigger because we know from changed being true, that the trigger column position(s) has been affected by the drop column.

          If we took out the information kept by"changed", then we won't be sure if we should drop the trigger because we are not certain if any trigger column's position changed because of the drop column. We could change the
          code inside the if to alway drop the trigger inside the if (j == refColLen) ie even if none of the column positions got impacted, then rearrange the column positions if needed and then recreate the trigger. ALTER TABLE DROP COLUMN should be a rare operation in production environment and hence it choosing to drop the unaffected trigger and recreating them should not be a big drawback. Or we can be more intelligent about dropping the trigger by doing so only if the trigger column positions have changed.

          So, we can either add some comments to the existing code to improve the readability of the if codition or we can get rid of the "changed" variable and simply drop the trigger, rearrange it's column positions if needed and recrete the trigger. Existing code avoids unnecessary drop and recreate of the trigger if the trigger columns are not affected. But your suggestion makes the code more readable. I am fine with either of the 2 choices.

          3)
          changedColPositionInTriggerAction is set but not used anywhere?
          Shouldn't the positions in referencedColsInTriggerAction be adjusted
          above droppedColumnPosition, too?

          For trigger reference columns, we can't simply do what is being done for trigger columns because trigger action sql gets changed to look at the columns available through the REFERENCING clause to be accessed by their column positions. Hence, just changing the column positions of affected referenced columns in systrigges is not enough. We need to regenerate the trigger action too.I am looking at tackling the regeneration of the trigger action for such a case through the jira DERBY-4984 which I am working on currently.

          4)You are correct about my comment being incorrect in AlterTableTest. I will fix that why I do my checkin for DERBY-4984

          Show
          Mamta A. Satoor added a comment - Dag, thanks for taking the time to review the patch. You comments/questions were as follows Comments on AlterTableConstantAction 1)You are right about following. This was a pre-existing comment. I will get rid of it because what we are doing follows the SQL standards. // otherwsie there would be unexpected behaviors Apart from the typo, is this not SQL requirement? 2) The "change" variable can probably be eliminated, since this test is repeated over again at end of loop: The code about the "change" variable is pre-existing too. Following is why I think it may have been coded this way. Inside the "if (j == refColLen && changed)" true code, we drop the trigger and then adjust the column positions of the affected trigger columns and then recreate the trigger with correct column positions. We drop the trigger because we know from changed being true, that the trigger column position(s) has been affected by the drop column. If we took out the information kept by"changed", then we won't be sure if we should drop the trigger because we are not certain if any trigger column's position changed because of the drop column. We could change the code inside the if to alway drop the trigger inside the if (j == refColLen) ie even if none of the column positions got impacted, then rearrange the column positions if needed and then recreate the trigger. ALTER TABLE DROP COLUMN should be a rare operation in production environment and hence it choosing to drop the unaffected trigger and recreating them should not be a big drawback. Or we can be more intelligent about dropping the trigger by doing so only if the trigger column positions have changed. So, we can either add some comments to the existing code to improve the readability of the if codition or we can get rid of the "changed" variable and simply drop the trigger, rearrange it's column positions if needed and recrete the trigger. Existing code avoids unnecessary drop and recreate of the trigger if the trigger columns are not affected. But your suggestion makes the code more readable. I am fine with either of the 2 choices. 3) changedColPositionInTriggerAction is set but not used anywhere? Shouldn't the positions in referencedColsInTriggerAction be adjusted above droppedColumnPosition, too? For trigger reference columns, we can't simply do what is being done for trigger columns because trigger action sql gets changed to look at the columns available through the REFERENCING clause to be accessed by their column positions. Hence, just changing the column positions of affected referenced columns in systrigges is not enough. We need to regenerate the trigger action too.I am looking at tackling the regeneration of the trigger action for such a case through the jira DERBY-4984 which I am working on currently. 4)You are correct about my comment being incorrect in AlterTableTest. I will fix that why I do my checkin for DERBY-4984
          Hide
          Mamta A. Satoor added a comment -

          I have backported the changes to 10.7 codeline after having run the derbyall and junit suite run successfully after the merge. If Dag's comments require any further changes, then I will put them both into trunk and 10.7.

          Show
          Mamta A. Satoor added a comment - I have backported the changes to 10.7 codeline after having run the derbyall and junit suite run successfully after the merge. If Dag's comments require any further changes, then I will put them both into trunk and 10.7.
          Hide
          Dag H. Wanvik added a comment -

          Thanks, Mamta! As for the "change" variable, since this is old code, I'm ok with keeping it, your call
          "Or we can be more intelligent about dropping the trigger by doing so only if the trigger column positions have changed. " This was my thought, yes, since we do compare the positions over again.

          Thanks also for explaining the missing piece being handled as part for DERBY-4984, great that this is being fixed!

          Show
          Dag H. Wanvik added a comment - Thanks, Mamta! As for the "change" variable, since this is old code, I'm ok with keeping it, your call "Or we can be more intelligent about dropping the trigger by doing so only if the trigger column positions have changed. " This was my thought, yes, since we do compare the positions over again. Thanks also for explaining the missing piece being handled as part for DERBY-4984 , great that this is being fixed!
          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.

            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