|
Yip Ng made changes - 14/Sep/06 06:33 AM
[
Permlink
| « Hide
]
Mamta A. Satoor added a comment - 14/Sep/06 04:08 PM
It looks like at alter table add column/drop column time, we need to go update the column "COLUMNS" in SYSCOLPERMS to incorporate the change in the table's column structure.
Yip Ng made changes - 15/Sep/06 04:56 PM
Yip Ng made changes - 15/Sep/06 05:17 PM
Bryan Pendleton made changes - 23/Sep/06 03:54 AM
Bryan Pendleton made changes - 23/Sep/06 03:25 PM
At a very high level, it seems like we need to:
- run through each row in SYSCOLPERMS and call FormatableBitSet.grow(1) on the COLUMNS column - clear the PermissionsCache so that in-memory descriptors will get re-read The clearing of the PermissionsCache seems like it should happen in DataDictionaryImpl.clearCaches. Thanks for sharing your findings from ALTER TABLE DROP COLUMN, Bryan. I will investigate further into this Jira entry to see how
1)SYSCOLPERMS should be updated and 2)clear the permission cache so that we don't use stale column permission descriptors.
Mamta A. Satoor made changes - 25/Sep/06 05:21 PM
It looks like the permission cache is handled differently than the other descriptor caches. When DataDictionary's startWriting() is called, the other caches will be *ALL* cleared out except for the permission cache, where the actual removal of its cached items are done in various DDL contant actions(DROP TABLE/VIEW, etc.) and it only removes those associated cached permission descriptor(s) and not *ALL* of them. So it seems like the permission cache clearing was left out intentionally in clearCaches(). It would be helpful to know if this is indeed the case.
Just wanted to mention that I was distracted by other work during last few days but have started working on this jira entry again and making progress. Hope to have something to propose by early next week.
I have attached a patch as DERBY1846_V1_diff_AddColumnAndGrantRevoke.txt
The output of svn stat -q is attached as DERBY1846_V1_stat_AddColumnAndGrantRevoke.txt To recap the problem, in SQL Authorization mode, when a new column is added to a table, the rows in SYSCOLPERMS for the table in question were not getting updated to incorporate the new column. This caused ASSERT failure when a non-table owner attempted to select the new column. Some background information on system table involved: SYSCOLPERMS keeps track of column level privileges on a given table. One of the columns in SYSCOLPERMS is "COLUMNS" and it has a bit map to show which columns have the given permission granted on them. When a new column is added to the user table, the "COLUMNS" need to be expanded by one bit and that bit should be initialized to zero since no privileges have been granted on that column at the ALTER TABLE...ADD COLUMN time. I have fixed this problem by having AlterTableConstantAction.addNewColumnToTable call the new method in DataDictionary called updateSYSCOLPERMSforAddColumnToUserTable. At this point, we know of only the TableDescriptor's uuid which can help us determine all the rows in SYSCOLPERMS for that given table uuid. I get ColPermsDescriptor for each one of those rows and then use the ColPermsDescriptor's uuid to update the "COLUMNS" column so SYSCOLPERMS is aware of the newly added column in user table. This fixes the problem because at the time of SELECT, when we do privilege lookup in SYSCOLPERMS, we have info on the newly added column. I have added few tests for this Jira entry in lang/grantRevokeDDL.sql The derbyall suite ran fine on Windows XP with Sun's jdk 1.4 Any feedback will be greatly appreciated.
Mamta A. Satoor made changes - 03/Oct/06 06:24 PM
Mamta A. Satoor made changes - 03/Oct/06 06:25 PM
Hi Mamta, I took a look at your patch and it makes sense to me. A handful of questions:
1) I was interested in why you chose to put the new logic into DataDictionaryImpl as opposed to, perhaps, TablePrivilegeInfo. 2) I was wondering whether you thought that updateSYSCOLPERMSforAddColumnToUserTable() could also be used, in the future, as part of solving possible future reuse of this routine might run into any complications trying to share the code. 3) I was confused by this part of your change to DataDictionaryImpl (see below). Is this a necessary part of the change, or is it some cleanup that you were doing as part of working in this class? @@ -2528,7 +2606,6 @@ { ExecRow curRow; PermissionsDescriptor perm; - ExecIndexRow newKey; TabInfoImpl ti = getNonCoreTI(SYSTABLEPERMS_CATALOG_NUM); SYSTABLEPERMSRowFactory rf = (SYSTABLEPERMSRowFactory) ti.getCatalogRowFactory(); @@ -2560,7 +2637,6 @@ { ExecRow curRow; PermissionsDescriptor perm; - ExecIndexRow newKey; TabInfoImpl ti = getNonCoreTI(SYSCOLPERMS_CATALOG_NUM); SYSCOLPERMSRowFactory rf = (SYSCOLPERMSRowFactory) ti.getCatalogRowFactory(); @@ -10223,9 +10299,7 @@ // Remove cached permissions data. The cache may hold permissions data for this key even if // the row in the permissions table is new. In that case the cache may have an entry indicating no // permissions - Cacheable cacheEntry = getPermissionsCache().findCached( perm); - if( cacheEntry != null) - getPermissionsCache().remove( cacheEntry); + removePermEntryInCache(perm); //If we are dealing with grant, then the caller does not need to send //any invalidation actions to anyone and hence return false Bryan, thanks for taking the time to review the code.
Let me answer attempt to answer your questions 1)Main reason behind putting the update system table code in DataDictionary was that for some reason, I thought DataDictionary class was the place to put all the code related to updating the system tables. 2)I think 3)I always tend to cleanup code around when I work on a Jira entry although it is not related to actual work that I am doing (wish, I did that at home, my house will look much cleaner). So, sorry about the confusion about the cleanup work that went into this patch. They were not related to the bug and I should have said something in my patch description about it.
committed patch to the trunk and marked fixed in 10.3.
m3_ibm142:148>svn commit Sending java\engine\org\apache\derby\iapi\sql\dictionary\DataDictionary.j ava Sending java\engine\org\apache\derby\impl\sql\catalog\DataDictionaryImpl. java Sending java\engine\org\apache\derby\impl\sql\catalog\SYSCOLPERMSRowFacto ry.java Sending java\engine\org\apache\derby\impl\sql\execute\AlterTableConstantA ction.java Sending java\testing\org\apache\derbyTesting\functionTests\master\grantRe vokeDDL.out Sending java\testing\org\apache\derbyTesting\functionTests\tests\lang\gra ntRevokeDDL.sql Transmitting file data ...... Committed revision 453352.
Mike Matrigali made changes - 05/Oct/06 08:16 PM
Mamta A. Satoor made changes - 06/Oct/06 06:08 AM
backported fix from trunk to 10.2 branch.
Mike Matrigali made changes - 06/Oct/06 09:28 PM
Bryan Pendleton made changes - 26/Dec/06 04:15 PM
I think there is a problem in this fix. I think that the problem is that
when DataDictionaryImpl.updateSYSCOLPERMSforAddColumnToUserTable updates the SYSCOLPERMS table, it updates the table by partial key value. That means that each time the updateRow() call is made, the COLUMNS column in SYSCOLPERMS is updated for *all* the SYSCOLPERMS in that particular table, not just for the particular SYSCOLPERMS row that we are working with at that instant. I think that the routine uses a partial key to find all the ColPermsDescriptor entries for this table, but when it updates those rows, I think it needs to use a full key, not a partial key. Here's a short script which I believe demonstrates the problem. Note that in the second fetch from SYSCOLPERMS, all the COLUMNS values have been changed to {0}. -bash-2.05b$ java -Dderby.database.sqlAuthorization=true org.apache.derby.tools.ij repro.sql ij version 10.3 ij> connect 'jdbc:derby:d1847;create=true'; WARNING 01J14: SQL authorization is being used without first enabling authentication. ij> create table t (a int, b int, c int); 0 rows inserted/updated/deleted ij> grant select (a) on t to first_user; 0 rows inserted/updated/deleted ij> grant update (b) on t to second_user; 0 rows inserted/updated/deleted ij> grant select (c) on t to third_user; 0 rows inserted/updated/deleted ij> select grantee, type, columns from sys.syscolperms; GRANTEE |&|COLUMNS -------------------------------------------------------------------------------------------------------------------------------------------------- FIRST_USER |s|{0} SECOND_USER |u|{1} THIRD_USER |s|{2} 3 rows selected ij> alter table t add column d int; 0 rows inserted/updated/deleted ij> select grantee, type, columns from sys.syscolperms; GRANTEE |&|COLUMNS -------------------------------------------------------------------------------------------------------------------------------------------------- FIRST_USER |s|{0} SECOND_USER |u|{0} THIRD_USER |s|{0} 3 rows selected ij> quit; -bash-2.05b$ cat repro.sql connect 'jdbc:derby:d1847;create=true'; create table t (a int, b int, c int); grant select (a) on t to first_user; grant update (b) on t to second_user; grant select (c) on t to third_user; select grantee, type, columns from sys.syscolperms; alter table t add column d int; select grantee, type, columns from sys.syscolperms; quit;
Bryan Pendleton made changes - 27/Dec/06 02:49 AM
Bryan Pendleton made changes - 27/Dec/06 05:03 PM
I believe that updateSYSCOLPERMSforAddColumnToUserTable
actually has the correct index row available, because it's just used that row to fetch the base table row to update. So I think the fix is as simple as passing that index row to the ti.updateRow() call and specifying the COLPERMSID_INDEX_NUM rather than the TABLEID_INDEX_NUM. Attached is d1847_useCPID_index.diff, a proposed patch for this issue which includes the above code change, and a simple new regression test based on the repro script I posted before. I'm running all the tests now, to see if this causes any other problems. Please review the change and let me know what you think.
Bryan Pendleton made changes - 27/Dec/06 05:08 PM
Bryan Pendleton made changes - 27/Dec/06 05:08 PM
Andrew McIntyre made changes - 30/Dec/06 08:27 AM
Hi Yip, thanks for the review! My derbyall and suites.All runs were clean,
so I've committed d1847_useCPID_index.diff to the trunk as revision 492919. I intend to merge this change to 10.2 and commit it there as well.
use_CPID follow-on patch applied to 10.2 as revision 493063.
Marking issue resolved.
Bryan Pendleton made changes - 05/Jan/07 04:18 PM
This issue is resolved and has not been updated in the last 12 months (since 24/Jan/07).
Dyre Tjeldvoll made changes - 24/Jan/08 01:10 PM
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Major
SELECT statement asserts with XJ001 when attempted to select a newly added column in SQL authorization mode
