Derby
  1. Derby
  2. DERBY-1489

Provide ALTER TABLE DROP COLUMN functionality

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.0.2.0, 10.0.2.1, 10.1.1.0, 10.1.2.1, 10.1.3.1, 10.2.1.6
    • Fix Version/s: 10.3.1.4
    • Component/s: Documentation, SQL
    • Labels:
      None
    • Urgency:
      Normal

      Description

      Provide a way to drop a column from an existing table. Possible syntax would be:

      ALTER TABLE tablename DROP COLUMN columnname CASCADE / RESTRICT;

      Feature should properly handle columns which are used in constraints, views, triggers, indexes, etc.

      1. dropColumn_v4_grant_tests.diff
        31 kB
        Bryan Pendleton
      2. dropColumn_v3_view_drop.diff
        30 kB
        Bryan Pendleton
      3. dropColumn_2.diff
        27 kB
        Bryan Pendleton
      4. drop_column_v8_restored_tests.diff
        60 kB
        Bryan Pendleton
      5. drop_column_v7_suggestions_from_mamta.diff
        23 kB
        Bryan Pendleton
      6. drop_column_v6_grant_not_implemented.diff
        36 kB
        Bryan Pendleton
      7. drop_column_v5_grant_tests.diff
        30 kB
        Bryan Pendleton

        Issue Links

          Activity

          Hide
          Andrew McIntyre added a comment -

          This issue has been resolved for over a year with no further movement. Closing.

          Show
          Andrew McIntyre added a comment - This issue has been resolved for over a year with no further movement. Closing.
          Hide
          Bryan Pendleton added a comment -

          Thanks very much to all the reviewers, and particularly to Mamta who checked several versions of this patch.

          I've committed the v8 version of the patch to subversion as revision 453420.

          I'm marking this issue resolved; for now, we have an undocumented new feature which does not work with sqlAuthorization mode. I hope to find time soon to work on the two followon issues: DERBY-1909 to fix the sqlAuthorization problem, and DERBY-1926 to provide documentation.

          Show
          Bryan Pendleton added a comment - Thanks very much to all the reviewers, and particularly to Mamta who checked several versions of this patch. I've committed the v8 version of the patch to subversion as revision 453420. I'm marking this issue resolved; for now, we have an undocumented new feature which does not work with sqlAuthorization mode. I hope to find time soon to work on the two followon issues: DERBY-1909 to fix the sqlAuthorization problem, and DERBY-1926 to provide documentation.
          Hide
          Bryan Pendleton added a comment -

          DERBY-1926 tracks the documentation for ALTER TABLE DROP COLUMN.

          Show
          Bryan Pendleton added a comment - DERBY-1926 tracks the documentation for ALTER TABLE DROP COLUMN.
          Hide
          Bryan Pendleton added a comment -

          Hi Mamta, thanks very much for the feedback. I didn't mean to rush you, just trying to find out who was reviewing.

          I definitely agree that we need good documentation for this feature, and I will file a new JIRA issue accordingly.

          I'm concerned that the current ALTER TABLE doc is already a bit unwieldy, and so I'm interested to get some suggestions about how we can rework that doc to make it more comprehensible and usable.

          I think we can deal with all those topics as part of the documentation JIRA.

          Show
          Bryan Pendleton added a comment - Hi Mamta, thanks very much for the feedback. I didn't mean to rush you, just trying to find out who was reviewing. I definitely agree that we need good documentation for this feature, and I will file a new JIRA issue accordingly. I'm concerned that the current ALTER TABLE doc is already a bit unwieldy, and so I'm interested to get some suggestions about how we can rework that doc to make it more comprehensible and usable. I think we can deal with all those topics as part of the documentation JIRA.
          Hide
          Mamta A. Satoor added a comment -

          I just finished reviewing the patch. It looks great, especially the comments in the code and in the tests. Will be very beneficial to next person in the line.

          My only feedback is do we have a jira entry for doc changes that need to go for this feature? It will be very helpful to outline how for instance unnamed NOT NULL constraint will not block DROP COLUMN RESTRICT, but an explicitly named CHECK constraint will blcok DROP COLUMN RESTRICT. Similarly, the behavior of indexes and DROP COLUMN RESTRICT should be documented. In short, what would block a RESTRICT and what would be allowed to pass through should be clearly documented so users are aware of the expected behavior.

          Other than that, thumbs up for this patch. Great Job.

          Show
          Mamta A. Satoor added a comment - I just finished reviewing the patch. It looks great, especially the comments in the code and in the tests. Will be very beneficial to next person in the line. My only feedback is do we have a jira entry for doc changes that need to go for this feature? It will be very helpful to outline how for instance unnamed NOT NULL constraint will not block DROP COLUMN RESTRICT, but an explicitly named CHECK constraint will blcok DROP COLUMN RESTRICT. Similarly, the behavior of indexes and DROP COLUMN RESTRICT should be documented. In short, what would block a RESTRICT and what would be allowed to pass through should be clearly documented so users are aware of the expected behavior. Other than that, thumbs up for this patch. Great Job.
          Hide
          Mamta A. Satoor added a comment -

          Bryan, I am reviewing the patch right now. Once I finish looking through the test changes, I will post some trivial comments from documentation point of view.

          Show
          Mamta A. Satoor added a comment - Bryan, I am reviewing the patch right now. Once I finish looking through the test changes, I will post some trivial comments from documentation point of view.
          Hide
          Bryan Pendleton added a comment -

          I propose to commit drop_column_v8_restored_tests.diff. If there are any
          reviewers currently examining this patch, please let me know. Unless any
          additional review comments are raised in the next 48 hours, I will commit the patch.

          Show
          Bryan Pendleton added a comment - I propose to commit drop_column_v8_restored_tests.diff. If there are any reviewers currently examining this patch, please let me know. Unless any additional review comments are raised in the next 48 hours, I will commit the patch.
          Hide
          Bryan Pendleton added a comment -

          I recovered the tests from a backup machine, and attached is
          'drop_column_v8_restored_tests.diff', which is the complete patch.

          Please have a look. Thanks!

          Show
          Bryan Pendleton added a comment - I recovered the tests from a backup machine, and attached is 'drop_column_v8_restored_tests.diff', which is the complete patch. Please have a look. Thanks!
          Hide
          Bryan Pendleton added a comment -

          OOPS! Patch 7 lost the tests. I messed up my "svn add" in my workspace. Hang on a bit...

          Show
          Bryan Pendleton added a comment - OOPS! Patch 7 lost the tests. I messed up my "svn add" in my workspace. Hang on a bit...
          Hide
          Bryan Pendleton added a comment -

          Attached is "drop_column_v7_suggestions_from_mamta.diff", which updates the
          previous patch proposal to respond to feedback from reviewers (thanks Mamta!)

          This patch contains more tests and more comments. It also contains two functional
          changes relative to the previous diff:

          • a warning message is now issued if a view is dropped indirectly as a result of
            revoking a privilege or dropping a column that the view is dependent on. This
            required updating the master file for grantRevokeDDL.out
          • the DROP COLUMN execution code now re-reads the table descriptor from the
            system catalogs after dropping all the dependent schema objects and before
            compressing the table, to ensure that the compressTable logic uses the most
            up-to-date table definition.

          Additional comments and feedback would be very appreciated!

          I think this patch may be ready to commit, so I'd really like to encourage anybody
          who has a chance, to give it a look.

          Show
          Bryan Pendleton added a comment - Attached is "drop_column_v7_suggestions_from_mamta.diff", which updates the previous patch proposal to respond to feedback from reviewers (thanks Mamta!) This patch contains more tests and more comments. It also contains two functional changes relative to the previous diff: a warning message is now issued if a view is dropped indirectly as a result of revoking a privilege or dropping a column that the view is dependent on. This required updating the master file for grantRevokeDDL.out the DROP COLUMN execution code now re-reads the table descriptor from the system catalogs after dropping all the dependent schema objects and before compressing the table, to ensure that the compressTable logic uses the most up-to-date table definition. Additional comments and feedback would be very appreciated! I think this patch may be ready to commit, so I'd really like to encourage anybody who has a chance, to give it a look.
          Hide
          Bryan Pendleton added a comment -

          Mamta, thanks again for the great review and the suggestions! They have been very interesting to pursue.

          I am intending to attach a revised patch later this weekend incorporating your suggestions, but I wanted
          to respond to a couple of your notes separately, as follows:

          1) Regarding the handling of indexes when dropping a column, there was a discussion in early July
          on the derby-dev list in which we concluded that CASCADE/RESTRICT should not consider indexes,
          but instead should always simply remove the column from any affected indexes, and drop the entire
          index if this column was the last column. Here's a pointer to the email discussion:
          http://www.nabble.com/forum/ViewPost.jtp?post=5254954&framed=y

          2) I'm glad you asked some questions about constraint handling because that caused me to write
          some more tests with PRIMARY and FOREIGN keys, and I found a problem in the case where the
          primary key, and the foreign key which references it, are both in the same table. The execution code
          was correctly cascading the delete, but during the table rebuild we were using a stale table
          descriptor and getting a "conglomerate not found" error. I changed the execution logic so that, after
          it has dropped all the affected PRIMARY and FOREIGN key constraints (and indexes) and before
          it rebuilds the base table, it re-reads the table descriptor from the system tables to ensure that it
          has the right table descriptor information. So I added a bunch more tests in this latest patch

          3) You asked a very interesting question: why is following if condition not required anymore?
          if (numRefCols > 1 || cd.getConstraintType() == DataDictionary.PRIMARYKEY_CONSTRAINT)

          I removed this "if" test when I was changing DROP COLUMN to properly handle UNIQUE constraints.

          This line of code is in the code path which is performing RESTRICT analysis, to see if the constraint
          that has been found should cause the DROP COLUMN to fail.

          The effect of the "if" test was to say that the only constraints that caused DROP COLUMN RESTRICT
          to fail were those that were either:

          • PRIMARY KEY constraints that referred to this column, or
          • other types of constraints that referred to this column, as well as some other column in this table

          When I added processing for UNIQUE constraints, I thought about modifying this "if" test to say

          cd.getConstraintType() == DataDictionary.UNIQUE_CONSTRAINT

          so that UNIQUE constraints that referred to only this column in this table would fail in RESTRICT mode,
          but the more I looked at this "if" test, the more I thought that the test was just unnecessary. We don't
          actually care whether the constraint affects 1 column or multiple columns, and we don't care whether
          it is a PRIMARYKEY_CONSTRAINT or some other sort of constraint. All we care about is that there
          is a constraint on this table which references this column, and the user has said RESTRICT.

          By removing the "if" test, the code interprets RESTRICT to apply to any such constraint, which I think
          is the right behavior.

          ===================

          I believe I've incorporated all your other suggestions into the revised patch. When my testing completes,
          I'll attach that patch, and look forward to additional feedback.

          Show
          Bryan Pendleton added a comment - Mamta, thanks again for the great review and the suggestions! They have been very interesting to pursue. I am intending to attach a revised patch later this weekend incorporating your suggestions, but I wanted to respond to a couple of your notes separately, as follows: 1) Regarding the handling of indexes when dropping a column, there was a discussion in early July on the derby-dev list in which we concluded that CASCADE/RESTRICT should not consider indexes, but instead should always simply remove the column from any affected indexes, and drop the entire index if this column was the last column. Here's a pointer to the email discussion: http://www.nabble.com/forum/ViewPost.jtp?post=5254954&framed=y 2) I'm glad you asked some questions about constraint handling because that caused me to write some more tests with PRIMARY and FOREIGN keys, and I found a problem in the case where the primary key, and the foreign key which references it, are both in the same table. The execution code was correctly cascading the delete, but during the table rebuild we were using a stale table descriptor and getting a "conglomerate not found" error. I changed the execution logic so that, after it has dropped all the affected PRIMARY and FOREIGN key constraints (and indexes) and before it rebuilds the base table, it re-reads the table descriptor from the system tables to ensure that it has the right table descriptor information. So I added a bunch more tests in this latest patch 3) You asked a very interesting question: why is following if condition not required anymore? if (numRefCols > 1 || cd.getConstraintType() == DataDictionary.PRIMARYKEY_CONSTRAINT) I removed this "if" test when I was changing DROP COLUMN to properly handle UNIQUE constraints. This line of code is in the code path which is performing RESTRICT analysis, to see if the constraint that has been found should cause the DROP COLUMN to fail. The effect of the "if" test was to say that the only constraints that caused DROP COLUMN RESTRICT to fail were those that were either: PRIMARY KEY constraints that referred to this column, or other types of constraints that referred to this column, as well as some other column in this table When I added processing for UNIQUE constraints, I thought about modifying this "if" test to say cd.getConstraintType() == DataDictionary.UNIQUE_CONSTRAINT so that UNIQUE constraints that referred to only this column in this table would fail in RESTRICT mode, but the more I looked at this "if" test, the more I thought that the test was just unnecessary. We don't actually care whether the constraint affects 1 column or multiple columns, and we don't care whether it is a PRIMARYKEY_CONSTRAINT or some other sort of constraint. All we care about is that there is a constraint on this table which references this column, and the user has said RESTRICT. By removing the "if" test, the code interprets RESTRICT to apply to any such constraint, which I think is the right behavior. =================== I believe I've incorporated all your other suggestions into the revised patch. When my testing completes, I'll attach that patch, and look forward to additional feedback.
          Hide
          Bryan Pendleton added a comment -

          The community seems comfortable with the idea of tracking the problems with DROP COLUMN and GRANTed column privileges as a separate JIRA entry, so I am going to go ahead and file that issue now.

          Show
          Bryan Pendleton added a comment - The community seems comfortable with the idea of tracking the problems with DROP COLUMN and GRANTed column privileges as a separate JIRA entry, so I am going to go ahead and file that issue now.
          Hide
          Bryan Pendleton added a comment -

          Thanks Mamta! This is very useful and helpful.

          Show
          Bryan Pendleton added a comment - Thanks Mamta! This is very useful and helpful.
          Hide
          Mamta A. Satoor added a comment -

          Bryan, thanks for the patch. I have some comments on it.

          ***************************************************************************************************************************************************************
          Comments for the engine changes
          ***************************************************************************************************************************************************************
          1)In ViewDescriptor.prepareToInvalide, there are some existing comments saying that REVOKE_PRIVILEGE_RESTRICT is going to throw an exception. Should we add some comments for DROP_COLUMN_RESTRICT too? I can see why you didn't add anything because by default, any invalidation that is not caught will endup in exception and hence probably no need to add comments for them.
          ********
          2)In AlterTableConstantAction, why is following if condition not required anymore?
          if (numRefCols > 1 || cd.getConstraintType() == DataDictionary.PRIMARYKEY_CONSTRAINT)
          ********
          3)Is keyword COLUMN optional in the following syntax?
          ALTER TABLE tablename DROP COLUMN columnname [CASCADE | RESTRICT]
          The reason I ask is there is a test in altertableDropColumn as follows
          +ij> alter table atdc_2 drop b;
          +ERROR 42X14: 'B' is not a column in table or VTI 'ATDC_2'.
          I thought the error message will be something different for missing keyword COLUMN/CONSTRAINT.
          ********
          4)I am wondering if we should change the error text for the drop column in sql authorization mode ie rather than saying (sqlAuthorization=true), say that we are in SQL Authorization mode or something along that line. But since this is temporary, probably not worth it.
          +ERROR 0A000: Feature not implemented: ALTER TABLE DROP COLUMN (sqlAuthorization=true).
          ***************************************************************************************************************************************************************

          ***************************************************************************************************************************************************************
          Comments for the test cases
          ***************************************************************************************************************************************************************
          1)If there is a check constraint defined on a column and drop column CASCADE is executed, should it drop the check constraint? In the new test file, I see an example for drop column restrict failing when there is a check constraint defined on it but I am not clear on what would happen for drop column cascade in this case.
          ********
          2)Also, I didn't quite understand the behavior when a column is dropped that participates in a multi-column index? It seems from the test that the column gets dropped but index is still around.
          Also, from the test, it looks like if the colum being dropped is the only column in the index, then the column and index both get dropped.
          ********
          3)Is there some work required for following comment in the new test?
          +-- FIXME: cascade/restrict processing doesn't currently consider indexes?
          It seems like your concern is correct because if there is a primary key/check constraint, the drop column fails. So, seems like it should fail for index too. I haven't checked what the SQL spec says.
          If we find that the behavior of drop column is different for different kinds of constraints, then some documentation on that will be very useful.
          ********
          4)With one level dependency, ie a view defined on a column. The column drop drops the view but there is no warning raised. In case of triggers, we generate a warning (copied following from the new test master). Should there be warning for drop of views/other constraints that get dropped automatically because of drop column cascade?
          +ij> alter table atdc_6 drop column b cascade;
          +0 rows inserted/updated/deleted
          +WARNING 01502: The trigger ATDC_6_TRIGGER_1 on table ATDC_6 has been dropped.
          ********
          5)Consider a case where a column is referenced by a view and that view is referenced by another view. What would happen if someone tries to
          drop that column with cascade behavior. Will it succeed, will both views get dropped too or will the drop fail?
          column->view1->view2
          drop column cascade will drop both views?
          ***************************************************************************************************************************************************************

          General comment
          1)Bryan, I think you already have this in your radar screen but I will bring it up, just in case. I think you are planning to add a new jira entry for DROP COLUMN and sql authorization if we decide to go ahead with the functionality proposed by this patch. Once that is done and before this patch gets committed, we should put the correct jira number in comments in altertableDropColumn.sql and in altertable.sql. Since we donot have a jira entry at this point, I see that we refer to it as DERBY-XXXX in the test comments. Also, in the new jira entry for DROP COLUMN in sql authorization mode, we should mention that altertableDropColumn.sql and in altertable.sql should be consolidated into altertable.sql.

          Show
          Mamta A. Satoor added a comment - Bryan, thanks for the patch. I have some comments on it. *************************************************************************************************************************************************************** Comments for the engine changes *************************************************************************************************************************************************************** 1)In ViewDescriptor.prepareToInvalide, there are some existing comments saying that REVOKE_PRIVILEGE_RESTRICT is going to throw an exception. Should we add some comments for DROP_COLUMN_RESTRICT too? I can see why you didn't add anything because by default, any invalidation that is not caught will endup in exception and hence probably no need to add comments for them. ******** 2)In AlterTableConstantAction, why is following if condition not required anymore? if (numRefCols > 1 || cd.getConstraintType() == DataDictionary.PRIMARYKEY_CONSTRAINT) ******** 3)Is keyword COLUMN optional in the following syntax? ALTER TABLE tablename DROP COLUMN columnname [CASCADE | RESTRICT] The reason I ask is there is a test in altertableDropColumn as follows +ij> alter table atdc_2 drop b; +ERROR 42X14: 'B' is not a column in table or VTI 'ATDC_2'. I thought the error message will be something different for missing keyword COLUMN/CONSTRAINT. ******** 4)I am wondering if we should change the error text for the drop column in sql authorization mode ie rather than saying (sqlAuthorization=true), say that we are in SQL Authorization mode or something along that line. But since this is temporary, probably not worth it. +ERROR 0A000: Feature not implemented: ALTER TABLE DROP COLUMN (sqlAuthorization=true). *************************************************************************************************************************************************************** *************************************************************************************************************************************************************** Comments for the test cases *************************************************************************************************************************************************************** 1)If there is a check constraint defined on a column and drop column CASCADE is executed, should it drop the check constraint? In the new test file, I see an example for drop column restrict failing when there is a check constraint defined on it but I am not clear on what would happen for drop column cascade in this case. ******** 2)Also, I didn't quite understand the behavior when a column is dropped that participates in a multi-column index? It seems from the test that the column gets dropped but index is still around. Also, from the test, it looks like if the colum being dropped is the only column in the index, then the column and index both get dropped. ******** 3)Is there some work required for following comment in the new test? +-- FIXME: cascade/restrict processing doesn't currently consider indexes? It seems like your concern is correct because if there is a primary key/check constraint, the drop column fails. So, seems like it should fail for index too. I haven't checked what the SQL spec says. If we find that the behavior of drop column is different for different kinds of constraints, then some documentation on that will be very useful. ******** 4)With one level dependency, ie a view defined on a column. The column drop drops the view but there is no warning raised. In case of triggers, we generate a warning (copied following from the new test master). Should there be warning for drop of views/other constraints that get dropped automatically because of drop column cascade? +ij> alter table atdc_6 drop column b cascade; +0 rows inserted/updated/deleted +WARNING 01502: The trigger ATDC_6_TRIGGER_1 on table ATDC_6 has been dropped. ******** 5)Consider a case where a column is referenced by a view and that view is referenced by another view. What would happen if someone tries to drop that column with cascade behavior. Will it succeed, will both views get dropped too or will the drop fail? column->view1->view2 drop column cascade will drop both views? *************************************************************************************************************************************************************** General comment 1)Bryan, I think you already have this in your radar screen but I will bring it up, just in case. I think you are planning to add a new jira entry for DROP COLUMN and sql authorization if we decide to go ahead with the functionality proposed by this patch. Once that is done and before this patch gets committed, we should put the correct jira number in comments in altertableDropColumn.sql and in altertable.sql. Since we donot have a jira entry at this point, I see that we refer to it as DERBY-XXXX in the test comments. Also, in the new jira entry for DROP COLUMN in sql authorization mode, we should mention that altertableDropColumn.sql and in altertable.sql should be consolidated into altertable.sql.
          Hide
          Bryan Pendleton added a comment -

          Attached is drop_column_v6_grant_not_implemented.diff, a patch proposal
          which I believe is ready for review and possibly for commit (depending on
          what turns up during review).

          This patch causes ALTER TABLE DROP COLUMN to throw a NOT
          IMPLEMENTED error if sqlAuthorization is true. Because of this, all of
          the DROP COLUMN tests are shifted to a new test script, rather than being
          placed in altertable.sql, which runs with sqlAuthorization. If we adopt this
          patch proposal, I'll file a new JIRA to track the issue of getting DROP COLUMN
          to work under sqlAuthorization, and I'll update the test scripts to indicate that
          the separate test script is temporary, and should be consolidated once that
          problem is fixed.

          Except for this change in the testing mechanics and the new NOT IMPLEMENTED
          check in the grammar, this patch proposal is the same as the previous few
          attached patches. derbyall passes with this patch proposal.

          Please have a look and tell me what you think.

          Show
          Bryan Pendleton added a comment - Attached is drop_column_v6_grant_not_implemented.diff, a patch proposal which I believe is ready for review and possibly for commit (depending on what turns up during review). This patch causes ALTER TABLE DROP COLUMN to throw a NOT IMPLEMENTED error if sqlAuthorization is true. Because of this, all of the DROP COLUMN tests are shifted to a new test script, rather than being placed in altertable.sql, which runs with sqlAuthorization. If we adopt this patch proposal, I'll file a new JIRA to track the issue of getting DROP COLUMN to work under sqlAuthorization, and I'll update the test scripts to indicate that the separate test script is temporary, and should be consolidated once that problem is fixed. Except for this change in the testing mechanics and the new NOT IMPLEMENTED check in the grammar, this patch proposal is the same as the previous few attached patches. derbyall passes with this patch proposal. Please have a look and tell me what you think.
          Hide
          Bryan Pendleton added a comment -

          Dan suggested perhaps throwing a NOT_IMPLEMENTED exception if the user
          tries to issue ALTER TABLE DROP COLUMN in sqlAuthorization mode.

          This seems pretty straightforward to do.

          One issue, though, is with testing. Currently, the entire altertable.sql test runs
          in sqlAuthorization mode. Therefore, it's hard to see how to provide some
          tests of DROP COLUMN which run when sqlAuthorization mode is off, and
          some other tests of DROP COLUMN which run when sqlAuthorization mode
          is on.

          Any suggestions as to how to provide reasonable tests in this situation? Should we:

          • put just a single DROP COLUMN test in altertable.sql, verifying that DROP COLUMN
            is rejected in sqlAuthorization mode
          • and then add a new test case, lang/altertableDropColumn.sql, to hold the
            tests that verify the proper operation of DROP COLUMN when sqlAuthorization
            is off?
          Show
          Bryan Pendleton added a comment - Dan suggested perhaps throwing a NOT_IMPLEMENTED exception if the user tries to issue ALTER TABLE DROP COLUMN in sqlAuthorization mode. This seems pretty straightforward to do. One issue, though, is with testing. Currently, the entire altertable.sql test runs in sqlAuthorization mode. Therefore, it's hard to see how to provide some tests of DROP COLUMN which run when sqlAuthorization mode is off, and some other tests of DROP COLUMN which run when sqlAuthorization mode is on. Any suggestions as to how to provide reasonable tests in this situation? Should we: put just a single DROP COLUMN test in altertable.sql, verifying that DROP COLUMN is rejected in sqlAuthorization mode and then add a new test case, lang/altertableDropColumn.sql, to hold the tests that verify the proper operation of DROP COLUMN when sqlAuthorization is off?
          Hide
          Bryan Pendleton added a comment -

          Thanks for the suggestion, Dan. I think that is a pretty interesting idea. I will have a look
          at the details of throwing an unimplemented exception in SQL Auth mode.

          I agree that it is desirable to get the current code into the trunk so that the community
          can start gaining experience with it.

          Show
          Bryan Pendleton added a comment - Thanks for the suggestion, Dan. I think that is a pretty interesting idea. I will have a look at the details of throwing an unimplemented exception in SQL Auth mode. I agree that it is desirable to get the current code into the trunk so that the community can start gaining experience with it.
          Hide
          Daniel John Debrunner added a comment -

          Is it possible to commit the alter table drop column patch (in trunk) and work on the permissions issues as a follow on?
          Since it would be the trunk it could either be left as broken in SQL authorization mode, or a quick check could be made and throw an unimplemented exception in sql auth mode. Then at least the base code wouldbe available for others to test easily.

          Show
          Daniel John Debrunner added a comment - Is it possible to commit the alter table drop column patch (in trunk) and work on the permissions issues as a follow on? Since it would be the trunk it could either be left as broken in SQL authorization mode, or a quick check could be made and throw an unimplemented exception in sql auth mode. Then at least the base code wouldbe available for others to test easily.
          Hide
          Bryan Pendleton added a comment -

          I think it will be easier to fix DERBY-1847 first, as fixing that problem
          will involving building the necessary infrastructure to enable repairing
          the SYSCOLPERMS entries during DROP COLUMN. The DROP COLUMN
          changes to SYSCOLPERMS will be a tad more involved than the ADD COLUMN
          changes to SYSCOLPERMS, but we can get most of the heavy lifting out
          of the way by fixing DERBY-1847 first.

          Show
          Bryan Pendleton added a comment - I think it will be easier to fix DERBY-1847 first, as fixing that problem will involving building the necessary infrastructure to enable repairing the SYSCOLPERMS entries during DROP COLUMN. The DROP COLUMN changes to SYSCOLPERMS will be a tad more involved than the ADD COLUMN changes to SYSCOLPERMS, but we can get most of the heavy lifting out of the way by fixing DERBY-1847 first.
          Hide
          Bryan Pendleton added a comment -

          It seems like DataDictionaryImpl.clearCaches() ought to clear the PermissionsCache, the
          same way it clears the other DD caches. By itself, this doesn't fix anything, but it does seem
          like it will have to be a building block for a solution.

          Show
          Bryan Pendleton added a comment - It seems like DataDictionaryImpl.clearCaches() ought to clear the PermissionsCache, the same way it clears the other DD caches. By itself, this doesn't fix anything, but it does seem like it will have to be a building block for a solution.
          Hide
          Bryan Pendleton added a comment -

          Thanks Yip! I agree, there is considerable similarity between these issues. I will follow the progress of DERBY-1847.

          Show
          Bryan Pendleton added a comment - Thanks Yip! I agree, there is considerable similarity between these issues. I will follow the progress of DERBY-1847 .
          Hide
          Yip Ng added a comment -

          It appears that DERBY-1847 has similar problem to what you described. Specifically, the "COLUMN" field of the SYS.SYSCOLPERMS needs to be updated at ALTER TABLE ADD/DROP COLUMN time when the system is in SQL authorization mode.

          Show
          Yip Ng added a comment - It appears that DERBY-1847 has similar problem to what you described. Specifically, the "COLUMN" field of the SYS.SYSCOLPERMS needs to be updated at ALTER TABLE ADD/DROP COLUMN time when the system is in SQL authorization mode.
          Hide
          Bryan Pendleton added a comment -

          Hmmm... The column permissions may be a bit tougher than I thought.
          Here's an interesting case:

          create table t (a int, b int, c int, d int);
          grant select(a,c,d) on t to bryan;
          alter table t drop column c;

          In this case, we want the existing permissions for columns "a" and "d"
          to be preserved after the drop of column "c"

          In fact, it seems like we may have to do some relatively tricky processing
          of the SYSCOLPERMS table during DROP COLUMN because the
          column permissions are recorded by column position number, and the
          column permission numbers can change during DROP COLUMN.

          Previously, I had thought that we just had to scan through SYSCOLPERMS
          and delete the corresponding rows, but now I see that we need to check
          the rows and re-write them to reflect the changed column position numbers
          as well as the dropped column.

          Show
          Bryan Pendleton added a comment - Hmmm... The column permissions may be a bit tougher than I thought. Here's an interesting case: create table t (a int, b int, c int, d int); grant select(a,c,d) on t to bryan; alter table t drop column c; In this case, we want the existing permissions for columns "a" and "d" to be preserved after the drop of column "c" In fact, it seems like we may have to do some relatively tricky processing of the SYSCOLPERMS table during DROP COLUMN because the column permissions are recorded by column position number, and the column permission numbers can change during DROP COLUMN. Previously, I had thought that we just had to scan through SYSCOLPERMS and delete the corresponding rows, but now I see that we need to check the rows and re-write them to reflect the changed column position numbers as well as the dropped column.
          Hide
          Bryan Pendleton added a comment -

          Linked this issue to DERBY-1854 because I'd like to add a similar test case to this issue to verify that the conglomerate ID matching is working correctly.

          Show
          Bryan Pendleton added a comment - Linked this issue to DERBY-1854 because I'd like to add a similar test case to this issue to verify that the conglomerate ID matching is working correctly.
          Hide
          Bryan Pendleton added a comment -

          I copied this comment from a different email discussion, involving some
          possible interactions between DERBY-655, DERBY-1854, and this bug:

          > There are a number of code paths in the server that use the
          > "bait/switch" method. Off the top of my head: some alter table
          > modify column paths, maybe drop column, bulk load to an empty table.
          > Were these also broken for this problem? Is the code to do the
          > syscat update shared, and thus this fixes all of them? Probably
          > worth adding some test cases.

          So, the current overall status of this issu is:
          1) Need to write logic to drop GRANTed column permissions when dropping a column
          2) Need to add test cases to see if the DERBY-655 regression affects DROP COLUMN

          Show
          Bryan Pendleton added a comment - I copied this comment from a different email discussion, involving some possible interactions between DERBY-655 , DERBY-1854 , and this bug: > There are a number of code paths in the server that use the > "bait/switch" method. Off the top of my head: some alter table > modify column paths, maybe drop column, bulk load to an empty table. > Were these also broken for this problem? Is the code to do the > syscat update shared, and thus this fixes all of them? Probably > worth adding some test cases. So, the current overall status of this issu is: 1) Need to write logic to drop GRANTed column permissions when dropping a column 2) Need to add test cases to see if the DERBY-655 regression affects DROP COLUMN
          Hide
          Bryan Pendleton added a comment -

          drop_column_v5_grant_tests.diff is an updated patch proposal
          which resolves the conflicts from DERBY-1491, but is otherwise
          unchanged from the v4 version of the patch proposal. It can be
          applied cleanly to the head of trunk and demonstrates the
          unsatisfactory interactions with GRANTed column permissions.

          Show
          Bryan Pendleton added a comment - drop_column_v5_grant_tests.diff is an updated patch proposal which resolves the conflicts from DERBY-1491 , but is otherwise unchanged from the v4 version of the patch proposal. It can be applied cleanly to the head of trunk and demonstrates the unsatisfactory interactions with GRANTed column permissions.
          Hide
          Bryan Pendleton added a comment -

          I investigated the dependency manager to see how it handles column
          permissions. It doesn't appear to track dependencies between column
          permissions and the underlying columns. I sent a message to the derby-dev
          list to inquire about this part of the GRANT implementation.

          I've been studying how DROP COLUMN handles triggers which mention this
          column, to see if that holds some clues for how to handle GRANTs which
          mention this column. DROP COLUMN locates such triggers by asking the
          DataDictionary for a list of all triggers on this table, then iterates through the
          list, asking each trigger which columns it references, and then either cascades
          the DROP to the affected trigger or fails the DROP, depending on whether the
          user said CASCADE or RESTRICT.

          Unfortunately, there don't appear to be analogous methods for column
          permissions in the data dictionary to allow retrieval of all the column permissions
          on this table, for example.

          So, for now, I don't have a way to handle GRANTed column permissions when
          dropping a column from a table, and this appears to be a blocker problem
          for this feature.

          Show
          Bryan Pendleton added a comment - I investigated the dependency manager to see how it handles column permissions. It doesn't appear to track dependencies between column permissions and the underlying columns. I sent a message to the derby-dev list to inquire about this part of the GRANT implementation. I've been studying how DROP COLUMN handles triggers which mention this column, to see if that holds some clues for how to handle GRANTs which mention this column. DROP COLUMN locates such triggers by asking the DataDictionary for a list of all triggers on this table, then iterates through the list, asking each trigger which columns it references, and then either cascades the DROP to the affected trigger or fails the DROP, depending on whether the user said CASCADE or RESTRICT. Unfortunately, there don't appear to be analogous methods for column permissions in the data dictionary to allow retrieval of all the column permissions on this table, for example. So, for now, I don't have a way to handle GRANTed column permissions when dropping a column from a table, and this appears to be a blocker problem for this feature.
          Hide
          Bryan Pendleton added a comment -

          DERBY-1729 has some good examples of how to write tests which add
          and remove permissions and check that the permissions come and go as expected.

          Show
          Bryan Pendleton added a comment - DERBY-1729 has some good examples of how to write tests which add and remove permissions and check that the permissions come and go as expected.
          Hide
          Bryan Pendleton added a comment -

          Attached is drop_column_v4_grant_tests.diff. This patch proposal is
          updated to the latest trunk, to resolve some conflicts following the commit
          of DERBY-119 and DERBY-1583. The patch also contains some new tests
          which start to investigate the interaction of DROP COLUMN and column permissions.

          Unfortunately, the tests fail; the current behavior is that DROP COLUMN is not
          properly aware of granted column permissions. This, I think, confirms the
          suspicions that were raised earlier
          http://issues.apache.org/jira/browse/DERBY-1489#action_12421197

          The next step will be to investigate the dependency manager handling of
          column permissions with respect to dropping a column.

          Show
          Bryan Pendleton added a comment - Attached is drop_column_v4_grant_tests.diff. This patch proposal is updated to the latest trunk, to resolve some conflicts following the commit of DERBY-119 and DERBY-1583 . The patch also contains some new tests which start to investigate the interaction of DROP COLUMN and column permissions. Unfortunately, the tests fail; the current behavior is that DROP COLUMN is not properly aware of granted column permissions. This, I think, confirms the suspicions that were raised earlier http://issues.apache.org/jira/browse/DERBY-1489#action_12421197 The next step will be to investigate the dependency manager handling of column permissions with respect to dropping a column.
          Hide
          Bryan Pendleton added a comment -

          Attached is dropColumn_v3_view_drop.diff, a patch proposal which supercedes
          the previous patch proposal. This patch proposal adds some changes to the
          dependency manager's handling of views so that DROP COLUMN CASCADE
          now is able to drop a dependent view.

          The new patch uses a technique similar to that used by REVOKE PRIVILEGE
          to cause the view to be dropped when CASCADE is specified. The differences
          between this patch and the previous patch are as follows:

          • Added the new field DependencyManager.DROP_COLUMN_RESTRICT
          • ViewDescriptor now includes handling for DROP_COLUMN and
            DROP_COLUMN_RESTRICT: if RESTRICT is specified, a dependent view
            causes the DROP COLUMN to fail; if RESTRICT is not specified, a
            dependent view is automatically dropped when the column is dropped.
          • SPSDescriptor, which was the only other user of DependencyManager.DROP_COLUMN,
            treats DROP_COLUMN and DROP_COLUMN_RESTRICT identically.
          • When AlterTableConstantAction calls the DependencyManager, it passes
            either DROP_COLUMN or DROP_COLUMN_RESTRICT, depending on
            what the user specified.

          Tests and test output is also updated to match the new correct results.

          This patch is not ready for commit yet, because I need to address the interactions
          with GRANT/REVOKE, but that will require investigating DERBY-1583.

          In the meantime, feedback on this patch would be very welcome!

          Show
          Bryan Pendleton added a comment - Attached is dropColumn_v3_view_drop.diff, a patch proposal which supercedes the previous patch proposal. This patch proposal adds some changes to the dependency manager's handling of views so that DROP COLUMN CASCADE now is able to drop a dependent view. The new patch uses a technique similar to that used by REVOKE PRIVILEGE to cause the view to be dropped when CASCADE is specified. The differences between this patch and the previous patch are as follows: Added the new field DependencyManager.DROP_COLUMN_RESTRICT ViewDescriptor now includes handling for DROP_COLUMN and DROP_COLUMN_RESTRICT: if RESTRICT is specified, a dependent view causes the DROP COLUMN to fail; if RESTRICT is not specified, a dependent view is automatically dropped when the column is dropped. SPSDescriptor, which was the only other user of DependencyManager.DROP_COLUMN, treats DROP_COLUMN and DROP_COLUMN_RESTRICT identically. When AlterTableConstantAction calls the DependencyManager, it passes either DROP_COLUMN or DROP_COLUMN_RESTRICT, depending on what the user specified. Tests and test output is also updated to match the new correct results. This patch is not ready for commit yet, because I need to address the interactions with GRANT/REVOKE, but that will require investigating DERBY-1583 . In the meantime, feedback on this patch would be very welcome!
          Hide
          Bryan Pendleton added a comment -

          The only two places where DependencyManager.DROP_COLUMN appears
          to be used right now are:
          java/engine/org/apache/derby/impl/sql/execute/AlterTableConstantAction.java
          java/engine/org/apache/derby/iapi/sql/dictionary/SPSDescriptor.java

          Hopefully this means that the impact of creating a new DROP_COLUMN_RESTRICT
          would be confined to these two places (plus ViewDescriptor.java).

          Show
          Bryan Pendleton added a comment - The only two places where DependencyManager.DROP_COLUMN appears to be used right now are: java/engine/org/apache/derby/impl/sql/execute/AlterTableConstantAction.java java/engine/org/apache/derby/iapi/sql/dictionary/SPSDescriptor.java Hopefully this means that the impact of creating a new DROP_COLUMN_RESTRICT would be confined to these two places (plus ViewDescriptor.java).
          Hide
          Bryan Pendleton added a comment -

          The issue with views which are dependent on the column being dropped seems to involve
          the handling of the prepareToInvalidate() and makeInvalid() methods in the
          org.apache.derby.iapi.sql.dictionary.ViewDescriptor class.

          As a first experiment, I tried adding case statements for DependencyManager.DROP_COLUMN
          into both methods:
          in prepareToInvalidate, break
          in makeInvalid, call dropViewWork

          This partially fixes the problem: now ALTER TABLE DROP COLUMN drops any view which
          is dependent on that column.

          However, with this change, the view is dropped regardless of whether I say CASCADE or RESTRICT.

          I notice that ViewDescriptor has handling for REVOKE_PRIVILEGE and REVOKE_PRIVILEGE_RESTRICT,
          and I wonder: should I create a DependencyManager.DROP_COLUMN_RESTRICT and mimic
          the handling of REVOKE_PRIVILEGE?

          Show
          Bryan Pendleton added a comment - The issue with views which are dependent on the column being dropped seems to involve the handling of the prepareToInvalidate() and makeInvalid() methods in the org.apache.derby.iapi.sql.dictionary.ViewDescriptor class. As a first experiment, I tried adding case statements for DependencyManager.DROP_COLUMN into both methods: in prepareToInvalidate, break in makeInvalid, call dropViewWork This partially fixes the problem: now ALTER TABLE DROP COLUMN drops any view which is dependent on that column. However, with this change, the view is dropped regardless of whether I say CASCADE or RESTRICT. I notice that ViewDescriptor has handling for REVOKE_PRIVILEGE and REVOKE_PRIVILEGE_RESTRICT, and I wonder: should I create a DependencyManager.DROP_COLUMN_RESTRICT and mimic the handling of REVOKE_PRIVILEGE?
          Hide
          Bryan Pendleton added a comment -

          DERBY-1754 was a duplicate of DERBY-1583. Fixing link.

          Show
          Bryan Pendleton added a comment - DERBY-1754 was a duplicate of DERBY-1583 . Fixing link.
          Hide
          Bryan Pendleton added a comment -

          While trying to test the interaction between DROP COLUMN and GRANT/REVOKE, I encountered bug DERBY-1754.

          Show
          Bryan Pendleton added a comment - While trying to test the interaction between DROP COLUMN and GRANT/REVOKE, I encountered bug DERBY-1754 .
          Hide
          Tim Dudgeon added a comment -

          I've tested the basic drop column funtionality and it seems to be working. It is a bit slow though.
          Here are some apporximate benchmarks for this. I created a tables like these:

          CREATE TABLE a_table cd_id
          INT NOT NULL GENERATED ALWAYS AS IDENTITY
          (START WITH 1, INCREMENT BY 1)
          cd_structure BLOB, cd_smiles VARCHAR(4000)
          cd_fp1 INT,
          cd_fp2 INT,
          cd_fp3 INT,
          cd_fp4 INT,
          cd_fp5 INT,
          cd_fp6 INT,
          cd_fp7 INT,
          cd_fp8 INT,
          cd_fp9 INT,
          cd_fp10 INT,
          cd_fp11 INT,
          cd_fp12 INT,
          cd_fp13 INT,
          cd_fp14 INT,
          cd_fp15 INT,
          cd_fp16 INT
          )

          This is typical column for me. The BLOB column contains a long (approx 5K) string, all other columns are small.
          Then I loaded approx 60,000 rows into one of these tables, added a column and then dropped it. Then repeated the process in a different tables for 250,000 and 3,000,000 rows. I also did the first 2 in oracle for a comparison. The oracle db already had lots of data in it so things are probably tipped in Derby's favour. The times for dropping the column are shown in millis.

          Table size Derby Oracle
          60K 4886 1088
          250K 32108 13786
          3M 421521 ND

          Presumably the time is taken to delete all the data (though the column being dropped contains only nulls). But Derby is significantly slower than Oracle.

          Show
          Tim Dudgeon added a comment - I've tested the basic drop column funtionality and it seems to be working. It is a bit slow though. Here are some apporximate benchmarks for this. I created a tables like these: CREATE TABLE a_table cd_id INT NOT NULL GENERATED ALWAYS AS IDENTITY (START WITH 1, INCREMENT BY 1) cd_structure BLOB, cd_smiles VARCHAR(4000) cd_fp1 INT, cd_fp2 INT, cd_fp3 INT, cd_fp4 INT, cd_fp5 INT, cd_fp6 INT, cd_fp7 INT, cd_fp8 INT, cd_fp9 INT, cd_fp10 INT, cd_fp11 INT, cd_fp12 INT, cd_fp13 INT, cd_fp14 INT, cd_fp15 INT, cd_fp16 INT ) This is typical column for me. The BLOB column contains a long (approx 5K) string, all other columns are small. Then I loaded approx 60,000 rows into one of these tables, added a column and then dropped it. Then repeated the process in a different tables for 250,000 and 3,000,000 rows. I also did the first 2 in oracle for a comparison. The oracle db already had lots of data in it so things are probably tipped in Derby's favour. The times for dropping the column are shown in millis. Table size Derby Oracle 60K 4886 1088 250K 32108 13786 3M 421521 ND Presumably the time is taken to delete all the data (though the column being dropped contains only nulls). But Derby is significantly slower than Oracle.
          Hide
          Bryan Pendleton added a comment -

          Derby user Tim Dudgeon has been successfully experimenting with this patch,
          see: http://www.nabble.com/drop-column-functionality-tf2141055.html#a5910820

          Show
          Bryan Pendleton added a comment - Derby user Tim Dudgeon has been successfully experimenting with this patch, see: http://www.nabble.com/drop-column-functionality-tf2141055.html#a5910820
          Hide
          Bryan Pendleton added a comment -

          Set Fix Version to unknown; this patch won't be ready in time for 10.2.0.0

          Show
          Bryan Pendleton added a comment - Set Fix Version to unknown; this patch won't be ready in time for 10.2.0.0
          Hide
          Bryan Pendleton added a comment -

          Cleared patch available; I need to revise the patch to incorporate the suggestions from reviewers.

          Show
          Bryan Pendleton added a comment - Cleared patch available; I need to revise the patch to incorporate the suggestions from reviewers.
          Hide
          Bryan Pendleton added a comment -

          Thanks for the review, Rick! I will work on investigating the issues you raised.

          Show
          Bryan Pendleton added a comment - Thanks for the review, Rick! I will work on investigating the issues you raised.
          Hide
          Rick Hillegas added a comment -

          Hi Bryan: Thanks for this great feature. The parser changes look good: thanks for cleaning up the LOOKAHEADs in alterTableAction().

          1) I agree that it would be good to see some tests verifying that privilege descriptors are cleaned up.

          2) I'm curious about why ALTER TABLE DROP COLUMN CASCADE fails if a view mentions the column. From your comment in altertable.sql, it seems that this puzzles you too. It does look wrong to me.

          Show
          Rick Hillegas added a comment - Hi Bryan: Thanks for this great feature. The parser changes look good: thanks for cleaning up the LOOKAHEADs in alterTableAction(). 1) I agree that it would be good to see some tests verifying that privilege descriptors are cleaned up. 2) I'm curious about why ALTER TABLE DROP COLUMN CASCADE fails if a view mentions the column. From your comment in altertable.sql, it seems that this puzzles you too. It does look wrong to me.
          Hide
          Daniel John Debrunner added a comment -

          My guess is that some new code will need to be added in the privilege descriptor code. It probably doesn't handle an invalidation event corresponding to drop column, since drop column was not supported when that code was added.

          I think you are right that the existing drop column code should not need to change because of this. (famous last words

          Show
          Daniel John Debrunner added a comment - My guess is that some new code will need to be added in the privilege descriptor code. It probably doesn't handle an invalidation event corresponding to drop column, since drop column was not supported when that code was added. I think you are right that the existing drop column code should not need to change because of this. (famous last words
          Hide
          Bryan Pendleton added a comment -

          On the derby-dev list, Dan explained further: "with the dependency system it's not so much that code checks for dependent objects, more that code fires off an invalidation event (makeInvalid call) and the dependents either handle it or throw an exception to stop it."

          If I'm understanding this all correctly, then we don't really need to add any additional code to incorporate the new privileges feature's interaction with DROP COLUMN; we just need to add test cases to ensure that the existing dependency system is working properly.

          Show
          Bryan Pendleton added a comment - On the derby-dev list, Dan explained further: "with the dependency system it's not so much that code checks for dependent objects, more that code fires off an invalidation event (makeInvalid call) and the dependents either handle it or throw an exception to stop it." If I'm understanding this all correctly, then we don't really need to add any additional code to incorporate the new privileges feature's interaction with DROP COLUMN; we just need to add test cases to ensure that the existing dependency system is working properly.
          Hide
          Daniel John Debrunner added a comment -

          Bryan wrote:


          2) DROP COLUMN needs to automatically revoke any privileges granted on the column. We need some code in the dropColumnFromTable() method in AlterTableConstantAction which looks for privileges granted on this column, and if there are any, automatically revoke those privileges.


          That should also be driven by the dependency system. A grant privilege on a column should be dependent on the column.
          When the column is dropped it fires off a invalidation event and the privilege should drop itself .

          Show
          Daniel John Debrunner added a comment - Bryan wrote: 2) DROP COLUMN needs to automatically revoke any privileges granted on the column. We need some code in the dropColumnFromTable() method in AlterTableConstantAction which looks for privileges granted on this column, and if there are any, automatically revoke those privileges. That should also be driven by the dependency system. A grant privilege on a column should be dependent on the column. When the column is dropped it fires off a invalidation event and the privilege should drop itself .
          Hide
          Bryan Pendleton added a comment -

          This comment captures the results of a discussion on the derby-dev list regarding the interaction of ALTER TABLE DROP COLUMN with the new security privileges functionality:

          1) DROP COLUMN RESTRICT does not need to consider any additional restrictions regarding privileges. The SQL spec does not appear to contain any text that stops the drop in restrict mode if privileges have been granted on the column. If an object (view, trigger, etc.) is dependent on a column level privilege then it should already be dependent on the column itself. There should be no need to go through the list of privileges to see what objects are dependent on them.

          2) DROP COLUMN needs to automatically revoke any privileges granted on the column. We need some code in the dropColumnFromTable() method in AlterTableConstantAction which looks for privileges granted on this column, and if there are any, automatically revoke those privileges.

          3) Lastly, of course, tests of these various conditions need to be added.

          Show
          Bryan Pendleton added a comment - This comment captures the results of a discussion on the derby-dev list regarding the interaction of ALTER TABLE DROP COLUMN with the new security privileges functionality: 1) DROP COLUMN RESTRICT does not need to consider any additional restrictions regarding privileges. The SQL spec does not appear to contain any text that stops the drop in restrict mode if privileges have been granted on the column. If an object (view, trigger, etc.) is dependent on a column level privilege then it should already be dependent on the column itself. There should be no need to go through the list of privileges to see what objects are dependent on them. 2) DROP COLUMN needs to automatically revoke any privileges granted on the column. We need some code in the dropColumnFromTable() method in AlterTableConstantAction which looks for privileges granted on this column, and if there are any, automatically revoke those privileges. 3) Lastly, of course, tests of these various conditions need to be added.
          Hide
          Bryan Pendleton added a comment -

          Attachment dropColumn_2.diff continues the work done in DERBY-396's dropColumn_1.diff by adding a number of additional tests, and by modifying the db2Compatibility test.

          With this diff, derbyall passes cleanly.

          Feedback about the compiler changes in this diff would be wonderful, as would feedback about additional test cases to add.

          Show
          Bryan Pendleton added a comment - Attachment dropColumn_2.diff continues the work done in DERBY-396 's dropColumn_1.diff by adding a number of additional tests, and by modifying the db2Compatibility test. With this diff, derbyall passes cleanly. Feedback about the compiler changes in this diff would be wonderful, as would feedback about additional test cases to add.

            People

            • Assignee:
              Bryan Pendleton
              Reporter:
              Bryan Pendleton
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development