|
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. 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 . 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. 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 :-) 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. Thanks for the review, Rick! I will work on investigating the issues you raised.
Cleared patch available; I need to revise the patch to incorporate the suggestions from reviewers.
Set Fix Version to unknown; this patch won't be ready in time for 10.2.0.0
Derby user Tim Dudgeon has been successfully experimenting with this patch,
see: http://www.nabble.com/drop-column-functionality-tf2141055.html#a5910820 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. While trying to test the interaction between DROP COLUMN and GRANT/REVOKE, I encountered bug
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? 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). 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 In the meantime, feedback on this patch would be very welcome! 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 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. and remove permissions and check that the permissions come and go as expected. 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. drop_column_v5_grant_tests.diff is an updated patch proposal
which resolves the conflicts from 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. I copied this comment from a different email discussion, involving some
possible interactions between > 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 Linked this issue to
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. It appears that
Thanks Yip! I agree, there is considerable similarity between these issues. I will follow the progress of
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. I think it will be easier to fix
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 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. 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. 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? 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. 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. Thanks Mamta! This is very useful and helpful.
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.
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. 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. OOPS! Patch 7 lost the tests. I messed up my "svn add" in my workspace. Hang on a bit...
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! 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. 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.
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. 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. 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: This issue has been resolved for over a year with no further movement. Closing.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Major
Provide ALTER TABLE DROP COLUMN functionality
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.