Derby
  1. Derby
  2. DERBY-1539

A trigger should be dropped when a privilege required by the trigger is revoked.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.2.1.6
    • Fix Version/s: 10.2.1.6
    • Component/s: SQL
    • Labels:
      None
    • Urgency:
      Normal

      Description

      A trigger tracks its privileges requirements using Derby's Dependency Manager. If any one of those required privileges are revoked, the trigger should be dropped automatically.

      I am just creating a new jira entry here so it is easier to track sub items of DERBY-1330. Will link this Jira entry to DERBY-1330.

      See the functional spec attached to DERBY-1330

      1. DERBY1539V4diffDropTriggerOnRevokeRequiredPrivilege.txt
        78 kB
        Mamta A. Satoor
      2. DERBY1539V4statDropTriggerOnRevokeRequiredPrivilege.txt
        2 kB
        Mamta A. Satoor
      3. DERBY1539V4diffDropTriggerOnRevoke.txt
        30 kB
        Mamta A. Satoor
      4. DERBY1539V3diffDropTriggerOnRevoke.txt
        30 kB
        Mamta A. Satoor
      5. DERBY1539V3statDropTriggerOnRevoke.txt
        1 kB
        Mamta A. Satoor
      6. DERBY1539V2diffDropTriggerOnRevoke.txt
        24 kB
        Mamta A. Satoor
      7. DERBY1539V2statDropTriggerOnRevoke.txt
        0.8 kB
        Mamta A. Satoor
      8. DERBY1539V1hashCodeEqualsDiff.txt
        11 kB
        Mamta A. Satoor
      9. DERBY1539V1hashCodeEqualsStat.txt
        0.4 kB
        Mamta A. Satoor

        Issue Links

          Activity

          Hide
          Mamta A. Satoor added a comment -

          This(DERBY1539V1hashCodeEqualsDiff.txt) is a mini patch in preparation for more revoke privilege work that I am still working on.

          PermissionsDescriptor has 2 methods. namely, hashCode and equals. In the initial stages of Grant Revoke functionality, there was no UUID associated with the PermissionsDescriptor and hence these methods did not need to worry about UUID. But for revoke functionality, dependency manager is going to keep various object dependencies on PermissionsDescriptor using the UUID attached to the PermissionsDescriptor. Hence, when dependency manager works with PermissionsDescriptor, it only knows about UUID and doesn't know anything about tableUUID/routineUUID and hashCode & equals method need to be able to handle that. On the other hand, during compile state, when privilege requirements are collected for the statement, we only know of tableUUID/routineUUID and nothing about UUID of the PermissionsDescriptor. In order to handle these scenarios, I have made changes to the hashCode and equals method to look for tableUUID!=null or routineUUID!=null.

          I have added an extensive javadoc comment to hashCode method which explains the change in hashCode and equals method. I debated whether this comment should go as regular comment rather than javadoc comment. Let me know if anyone has any opinion about it.

          I am copying the comments from hashCode method here which hopefully will help understand the patch better

          • eg when will if condition below return true?
          • During sql statement execution time, one of the first things that
          • happen is the privilege checking. Every statement has a list of
          • privileges that should exist in order for the statement to execute
          • successfully. That list gets collected during statement compilation
          • time. During execution, the list knows of tableUUID of the table
          • on which the privilege is required but it doesn't know of the
          • UUID of the privilege. So, the if condition below for tableUUID!=null
          • will be true when privilege checking happens at statement execution
          • time.
          • eg when will if condition below return false?
          • When the dependency manager looks for dependents for a permission
          • descriptor, it doesn't know what is the permission descriptor's
          • tableUUID but it does know the UUID of the permission descriptor.
          • So, the if condition below for tableUUID!=null will be false when
          • dependency manager is looking for permission descriptor's dependents.
          • There are some instances where tableUUID and UUID are both known.
          • And it doesn't matter which code path gets picked up below
          • and hence I simply chose the tableUUID!=null as the catch bucket.
          • tableID and UUID will be both known when an existing permission
          • descriptor in the cache needs to be removed from cache because
          • a grant/revoke was issued and that has changed the allowed
          • permissions on the permission descriptor. This happens at the
          • end of DataDictionary.addRemovePermissionsDescriptor method.

          Can someone please review this patch and commit if everything looks good? I ran the derbyall last night and it ran fine with no new diffs.

          Show
          Mamta A. Satoor added a comment - This(DERBY1539V1hashCodeEqualsDiff.txt) is a mini patch in preparation for more revoke privilege work that I am still working on. PermissionsDescriptor has 2 methods. namely, hashCode and equals. In the initial stages of Grant Revoke functionality, there was no UUID associated with the PermissionsDescriptor and hence these methods did not need to worry about UUID. But for revoke functionality, dependency manager is going to keep various object dependencies on PermissionsDescriptor using the UUID attached to the PermissionsDescriptor. Hence, when dependency manager works with PermissionsDescriptor, it only knows about UUID and doesn't know anything about tableUUID/routineUUID and hashCode & equals method need to be able to handle that. On the other hand, during compile state, when privilege requirements are collected for the statement, we only know of tableUUID/routineUUID and nothing about UUID of the PermissionsDescriptor. In order to handle these scenarios, I have made changes to the hashCode and equals method to look for tableUUID!=null or routineUUID!=null. I have added an extensive javadoc comment to hashCode method which explains the change in hashCode and equals method. I debated whether this comment should go as regular comment rather than javadoc comment. Let me know if anyone has any opinion about it. I am copying the comments from hashCode method here which hopefully will help understand the patch better eg when will if condition below return true? During sql statement execution time, one of the first things that happen is the privilege checking. Every statement has a list of privileges that should exist in order for the statement to execute successfully. That list gets collected during statement compilation time. During execution, the list knows of tableUUID of the table on which the privilege is required but it doesn't know of the UUID of the privilege. So, the if condition below for tableUUID!=null will be true when privilege checking happens at statement execution time. eg when will if condition below return false? When the dependency manager looks for dependents for a permission descriptor, it doesn't know what is the permission descriptor's tableUUID but it does know the UUID of the permission descriptor. So, the if condition below for tableUUID!=null will be false when dependency manager is looking for permission descriptor's dependents. There are some instances where tableUUID and UUID are both known. And it doesn't matter which code path gets picked up below and hence I simply chose the tableUUID!=null as the catch bucket. tableID and UUID will be both known when an existing permission descriptor in the cache needs to be removed from cache because a grant/revoke was issued and that has changed the allowed permissions on the permission descriptor. This happens at the end of DataDictionary.addRemovePermissionsDescriptor method. Can someone please review this patch and commit if everything looks good? I ran the derbyall last night and it ran fine with no new diffs.
          Hide
          Daniel John Debrunner added a comment -

          WIll a Permissions descriptor now change its hashCode during its lifetime? From the code it looks like that's a possibility.
          I think that is not typical behaviour for objects and will lead to bugs that are hard to track down. Fro example changing the
          hashCode() return while an object is in a cache or hashtable is not supported, the object will become "lost" in the hash table.

          Show
          Daniel John Debrunner added a comment - WIll a Permissions descriptor now change its hashCode during its lifetime? From the code it looks like that's a possibility. I think that is not typical behaviour for objects and will lead to bugs that are hard to track down. Fro example changing the hashCode() return while an object is in a cache or hashtable is not supported, the object will become "lost" in the hash table.
          Hide
          Mamta A. Satoor added a comment -

          I want to start out by saying that this is my first time ever working with hashCode so I am new to this part of the code and hence new to the terminology. Having said that, I debugged through permissions caching and found that Permission Descriptor does not change its hashCode during its lifetime in the cache. I did my debugging for a TablePermissionsDescriptor and same logic should apply to ColPermsDescriptor and RoutinePermsDescriptor.

          Following are more details from debugging
          1)When a statement such as create view is compiled, a list of privileges are collected for it. At this point, we only know of tableUUID and grantee and the required privilege type and know nothing about the Permission Descriptor's UUID since we haven't gone to system tables yet.
          During the statement's execution phase, one required privilege is taken at a time. For each one of them, we first create a Permission Descriptor using the tableUUID, permission type and grantee as PUBLIC. We find if there is such a granted permission by going to system tables and if yes, then we get that permission's UUID and put it in the Permission Descriptor object and save it in the hash table using the tableUUID and grantee as the hash key(so the entry in the has table will have both UUID and tableUUID) and we move on to the next required privilege.
          If there is no PUBLIC level privilege found, then the Permission Descriptor with tableUUID, grantee as PUBLIC and no UUID is added to the hash table and we look for the privilege at the user specific level in the permissions system tables.
          If we find a privilege granted at the user level, then we add a Permission Descriptor with the UUID for that privilege(found from the system table), tableUUID and grantee as the actual user in the hash table and we move on to the next required privilege. Ofcourse, if no privilege found at PUBLIC or user level, there will be an exception thrown.
          So, at the end of the statement execution, the hash table would have permissions descriptors with or without UUIDs depending on whether a permission was granted at the PUBLIC or user level. But in both cases, the hashkey used is based on tableUUID and grantee.

          2)Next, when revoke privilege is called, dependency manager has access to the UUID of the privilege. It looks in the hashtable using UUID for the haskey(we do not know the tableUUID at this point and hence have to reply on UUID. This is the code change that I made in this patch). Since the earlier entries in hashtable is step 1) above were based on the hashcode of tableUUID and grantee, we will not find the entry in the hash table based on the UUID. The cahing code makes an entry in the hashtable using the hashkey based on UUID and as part of the remaining logic in the caching code for a new entry, we goto system tables and get the remaining information about Permission Descriptor which includes tableUUID, grantee, permission type etc. This happens in PermssionsCacheable.setIdentity. During this time, caching code finds that there is already an entry in the hashtable for the tableUUID and grantee(which were just found from the system table) and hence it uses the existing hashtable entry and removes the entry that was made using UUID as the hashkey. So, this is what ensures that a Permission Descriptor does not change its hashCode during its lifetime in the cache and we continue to use tableUUID and grantee to get stuff from the cache or to add stuff into the cache.

          The stack trace of where we abandon the hashentry made using UUID and reuse the existing hasttable entry based on tableUUID and grantee is as follows
          PermissionsCacheable.setIdentity(Object) line: 66
          CachedItem.takeOnIdentity(CacheManager, CacheableFactory, Object, boolean, Object) line: 235
          Clock.addEntry(CachedItem, Object, boolean, Object) line: 796
          Clock.find(Object) line: 301
          DataDictionaryImpl.getPermissions(PermissionsDescriptor) line: 9796
          DataDictionaryImpl.getTablePermissions(UUID) line: 9790
          DDdependableFinder.getDependable(DataDictionary, UUID) line: 354
          DDdependableFinder.getDependable(UUID) line: 180
          BasicDependencyManager.getDependencyDescriptorList(List) line: 1190
          BasicDependencyManager.getDependents(Provider) line: 1370
          BasicDependencyManager.coreInvalidateFor(Provider, int, LanguageConnectionContext) line: 247
          BasicDependencyManager.invalidateFor(Provider, int, LanguageConnectionContext) line: 224
          TablePermsDescriptor.sendInvalidationMessages(DependencyManager, LanguageConnectionContext) line: 194
          DataDictionaryImpl.addRemovePermissionsDescriptor(boolean, PermissionsDescriptor, String, TransactionController) line: 10004
          TablePrivilegeInfo.executeGrantRevoke(Activation, boolean, List) line: 131
          GrantRevokeConstantAction.executeConstantAction(Activation) line: 61
          MiscResultSet.open() line: 56
          GenericPreparedStatement.execute(Activation, boolean, long) line: 357
          EmbedStatement.executeStatement(Activation, boolean, boolean) line: 1181
          EmbedStatement.execute(String, boolean, boolean, int, int[], String[]) line: 584
          EmbedStatement.execute(String) line: 516
          ij.executeImmediate(String) line: 313
          utilMain14(utilMain).doCatch(String) line: 433
          utilMain14(utilMain).go(LocalizedInput[], LocalizedOutput, Properties) line: 310
          Main14(Main).go(LocalizedInput, LocalizedOutput, Properties) line: 207
          Main.mainCore(String[], Main) line: 173
          Main14.main(String[]) line: 55
          ij.main(String[]) line: 60

          Dan, I hope this answers your question.

          Show
          Mamta A. Satoor added a comment - I want to start out by saying that this is my first time ever working with hashCode so I am new to this part of the code and hence new to the terminology. Having said that, I debugged through permissions caching and found that Permission Descriptor does not change its hashCode during its lifetime in the cache. I did my debugging for a TablePermissionsDescriptor and same logic should apply to ColPermsDescriptor and RoutinePermsDescriptor. Following are more details from debugging 1)When a statement such as create view is compiled, a list of privileges are collected for it. At this point, we only know of tableUUID and grantee and the required privilege type and know nothing about the Permission Descriptor's UUID since we haven't gone to system tables yet. During the statement's execution phase, one required privilege is taken at a time. For each one of them, we first create a Permission Descriptor using the tableUUID, permission type and grantee as PUBLIC. We find if there is such a granted permission by going to system tables and if yes, then we get that permission's UUID and put it in the Permission Descriptor object and save it in the hash table using the tableUUID and grantee as the hash key(so the entry in the has table will have both UUID and tableUUID) and we move on to the next required privilege. If there is no PUBLIC level privilege found, then the Permission Descriptor with tableUUID, grantee as PUBLIC and no UUID is added to the hash table and we look for the privilege at the user specific level in the permissions system tables. If we find a privilege granted at the user level, then we add a Permission Descriptor with the UUID for that privilege(found from the system table), tableUUID and grantee as the actual user in the hash table and we move on to the next required privilege. Ofcourse, if no privilege found at PUBLIC or user level, there will be an exception thrown. So, at the end of the statement execution, the hash table would have permissions descriptors with or without UUIDs depending on whether a permission was granted at the PUBLIC or user level. But in both cases, the hashkey used is based on tableUUID and grantee. 2)Next, when revoke privilege is called, dependency manager has access to the UUID of the privilege. It looks in the hashtable using UUID for the haskey(we do not know the tableUUID at this point and hence have to reply on UUID. This is the code change that I made in this patch). Since the earlier entries in hashtable is step 1) above were based on the hashcode of tableUUID and grantee, we will not find the entry in the hash table based on the UUID. The cahing code makes an entry in the hashtable using the hashkey based on UUID and as part of the remaining logic in the caching code for a new entry, we goto system tables and get the remaining information about Permission Descriptor which includes tableUUID, grantee, permission type etc. This happens in PermssionsCacheable.setIdentity. During this time, caching code finds that there is already an entry in the hashtable for the tableUUID and grantee(which were just found from the system table) and hence it uses the existing hashtable entry and removes the entry that was made using UUID as the hashkey. So, this is what ensures that a Permission Descriptor does not change its hashCode during its lifetime in the cache and we continue to use tableUUID and grantee to get stuff from the cache or to add stuff into the cache. The stack trace of where we abandon the hashentry made using UUID and reuse the existing hasttable entry based on tableUUID and grantee is as follows PermissionsCacheable.setIdentity(Object) line: 66 CachedItem.takeOnIdentity(CacheManager, CacheableFactory, Object, boolean, Object) line: 235 Clock.addEntry(CachedItem, Object, boolean, Object) line: 796 Clock.find(Object) line: 301 DataDictionaryImpl.getPermissions(PermissionsDescriptor) line: 9796 DataDictionaryImpl.getTablePermissions(UUID) line: 9790 DDdependableFinder.getDependable(DataDictionary, UUID) line: 354 DDdependableFinder.getDependable(UUID) line: 180 BasicDependencyManager.getDependencyDescriptorList(List) line: 1190 BasicDependencyManager.getDependents(Provider) line: 1370 BasicDependencyManager.coreInvalidateFor(Provider, int, LanguageConnectionContext) line: 247 BasicDependencyManager.invalidateFor(Provider, int, LanguageConnectionContext) line: 224 TablePermsDescriptor.sendInvalidationMessages(DependencyManager, LanguageConnectionContext) line: 194 DataDictionaryImpl.addRemovePermissionsDescriptor(boolean, PermissionsDescriptor, String, TransactionController) line: 10004 TablePrivilegeInfo.executeGrantRevoke(Activation, boolean, List) line: 131 GrantRevokeConstantAction.executeConstantAction(Activation) line: 61 MiscResultSet.open() line: 56 GenericPreparedStatement.execute(Activation, boolean, long) line: 357 EmbedStatement.executeStatement(Activation, boolean, boolean) line: 1181 EmbedStatement.execute(String, boolean, boolean, int, int[], String[]) line: 584 EmbedStatement.execute(String) line: 516 ij.executeImmediate(String) line: 313 utilMain14(utilMain).doCatch(String) line: 433 utilMain14(utilMain).go(LocalizedInput[], LocalizedOutput, Properties) line: 310 Main14(Main).go(LocalizedInput, LocalizedOutput, Properties) line: 207 Main.mainCore(String[], Main) line: 173 Main14.main(String[]) line: 55 ij.main(String[]) line: 60 Dan, I hope this answers your question.
          Hide
          Daniel John Debrunner added a comment -

          Thanks Mamta for looking into this - I'm still concerned that the object has the ability to change its hashcode and it's worrying when you say:

          "Since the earlier entries in hashtable is step 1) above were based on the hashcode of tableUUID and grantee, we will not find the entry in the hash table based on the UUID."

          Seems something is wrong when we do a lookup expecting to fail and then subsequent find the object during creation time. I'm looking at the code to see what is going on.

          Show
          Daniel John Debrunner added a comment - Thanks Mamta for looking into this - I'm still concerned that the object has the ability to change its hashcode and it's worrying when you say: "Since the earlier entries in hashtable is step 1) above were based on the hashcode of tableUUID and grantee, we will not find the entry in the hash table based on the UUID." Seems something is wrong when we do a lookup expecting to fail and then subsequent find the object during creation time. I'm looking at the code to see what is going on.
          Hide
          Daniel John Debrunner added a comment -

          Could you explain more what you mean by:
          "Hence, when dependency manager works with PermissionsDescriptor, it only knows about UUID"

          Why does the PermissionDescriptor the dependency manager is working with only have a UUID?

          Show
          Daniel John Debrunner added a comment - Could you explain more what you mean by: "Hence, when dependency manager works with PermissionsDescriptor, it only knows about UUID" Why does the PermissionDescriptor the dependency manager is working with only have a UUID?
          Hide
          Mamta A. Satoor added a comment -

          Dan, the answer to your question "Why does the PermissionDescriptor the dependency manager is working with only have a UUID? " is that the Dependency Manager while working on finding dependents calls DDdependableFinder.getDependable with just the UUID of the PermissionDescriptor(BTW, this is generic logic and not specific to PermissionDescriptor). And from that UUID, DDdependableFinder gets the PermissionDescriptor using dd.getTablePermissions (dependableObjectID); on line 354. The code for dd.getTablePermissions(UUID) goes through the hashing code to find out if a PermissionDescriptor for UUID alreadys exists in the hashtable(at this point in the code path, we do not know tableUUID and that is why I had to change hashCode to use UUID for generating the hash key).

          One way to get around this hashing key issue for revoke privileges would be to bypass the hash table and go straight to the system tables to get the PermissionDescriptor using the UUID. So, we will bypass hash table only in case of revoke privilege statement when the dependency manager is working on sending revoke related actions to the PermissionDescriptor's dependents.

          This can be implemented by changing following method in DataDictionary
          public TablePermsDescriptor getTablePermissions( UUID tablePermsUUID)
          throws StandardException

          { TablePermsDescriptor key = new TablePermsDescriptor( this, tablePermsUUID); return (TablePermsDescriptor) getPermissions( key); }

          New implementation for the method above will look as follows
          public TablePermsDescriptor getTablePermissions( UUID tablePermsUUID)
          throws StandardException

          { TablePermsDescriptor key = new TablePermsDescriptor( this, tablePermsUUID); return getUncachedTablePermsDescriptor(key); }

          With this change, we will not need the changes suggested by me on DERBY-1539, patch DERBY1539V1hashCodeEqualsDiff.txt
          The good thing about this change would be that we will not use 2 different hash codes for the same object. The bad thing is that for revoke privilege, dependency manager will need to always goto system tables for PermissionDescriptor before it can send actions to dependents.

          Show
          Mamta A. Satoor added a comment - Dan, the answer to your question "Why does the PermissionDescriptor the dependency manager is working with only have a UUID? " is that the Dependency Manager while working on finding dependents calls DDdependableFinder.getDependable with just the UUID of the PermissionDescriptor(BTW, this is generic logic and not specific to PermissionDescriptor). And from that UUID, DDdependableFinder gets the PermissionDescriptor using dd.getTablePermissions (dependableObjectID); on line 354. The code for dd.getTablePermissions(UUID) goes through the hashing code to find out if a PermissionDescriptor for UUID alreadys exists in the hashtable(at this point in the code path, we do not know tableUUID and that is why I had to change hashCode to use UUID for generating the hash key). One way to get around this hashing key issue for revoke privileges would be to bypass the hash table and go straight to the system tables to get the PermissionDescriptor using the UUID. So, we will bypass hash table only in case of revoke privilege statement when the dependency manager is working on sending revoke related actions to the PermissionDescriptor's dependents. This can be implemented by changing following method in DataDictionary public TablePermsDescriptor getTablePermissions( UUID tablePermsUUID) throws StandardException { TablePermsDescriptor key = new TablePermsDescriptor( this, tablePermsUUID); return (TablePermsDescriptor) getPermissions( key); } New implementation for the method above will look as follows public TablePermsDescriptor getTablePermissions( UUID tablePermsUUID) throws StandardException { TablePermsDescriptor key = new TablePermsDescriptor( this, tablePermsUUID); return getUncachedTablePermsDescriptor(key); } With this change, we will not need the changes suggested by me on DERBY-1539 , patch DERBY1539V1hashCodeEqualsDiff.txt The good thing about this change would be that we will not use 2 different hash codes for the same object. The bad thing is that for revoke privilege, dependency manager will need to always goto system tables for PermissionDescriptor before it can send actions to dependents.
          Hide
          Mamta A. Satoor added a comment -

          While looking at DataDictionaryImpl for something else, I found that there are very few descriptor types that get cached (looks like TableDescriptor and SPSDescriptor and PermissionsDescriptor are the only ones). For all the rest of the descriptors, we always goto system tables.

          For TableDescriptor and SPSDescriptor, DataDictionary getter methods check to see if Derby is in compile mode and the descriptors are fetched from the cache only if Derby is in compile mode. If not in compile mode, Derby goes to system tables to get TableDescriptor and SPSDescriptor. I am not sure why caching is not used in execute mode for these 2 descriptors, but if we followed the same logic for PermissionDescriptors as well, we will not have the hash key code problem with PermissionDescriptors during the revoke privilege execute phase.

          Does anyone have any opinion about this? If so, please let me know in next couple days. If not, I will go ahead and mimic the cache bypassing behavior for PermissionDescriptors.

          Show
          Mamta A. Satoor added a comment - While looking at DataDictionaryImpl for something else, I found that there are very few descriptor types that get cached (looks like TableDescriptor and SPSDescriptor and PermissionsDescriptor are the only ones). For all the rest of the descriptors, we always goto system tables. For TableDescriptor and SPSDescriptor, DataDictionary getter methods check to see if Derby is in compile mode and the descriptors are fetched from the cache only if Derby is in compile mode. If not in compile mode, Derby goes to system tables to get TableDescriptor and SPSDescriptor. I am not sure why caching is not used in execute mode for these 2 descriptors, but if we followed the same logic for PermissionDescriptors as well, we will not have the hash key code problem with PermissionDescriptors during the revoke privilege execute phase. Does anyone have any opinion about this? If so, please let me know in next couple days. If not, I will go ahead and mimic the cache bypassing behavior for PermissionDescriptors.
          Hide
          Daniel John Debrunner added a comment -

          Mamta, I understand your general comments about the dependency manager and not knowing the tableUUID etc, but it doesn't clear up my confusion.
          I'm trying to understand how executing a REVOKE statement like:

          REVOKE SELECT ON TABLE S1.T1 FROM dan

          relates to what you wrote initially:

          "But for revoke functionality, dependency manager is going to keep various object dependencies on PermissionsDescriptor using the UUID attached to the PermissionsDescriptor. Hence, when dependency manager works with PermissionsDescriptor, it only knows about UUID and doesn't know anything about tableUUID/routineUUID and hashCode & equals method need to be able to handle that."

          Maybe my question is, at what point in time during the execution of a REVOKE do you end up with a PermissionsDescriptor that only has been populated with its object UUID, and what permission does the PermissionsDescriptor represent?

          I think I'm still a little in the dark as to the overall problem this patch is addressing, rather than the specific of a cache lookup not working.

          Show
          Daniel John Debrunner added a comment - Mamta, I understand your general comments about the dependency manager and not knowing the tableUUID etc, but it doesn't clear up my confusion. I'm trying to understand how executing a REVOKE statement like: REVOKE SELECT ON TABLE S1.T1 FROM dan relates to what you wrote initially: "But for revoke functionality, dependency manager is going to keep various object dependencies on PermissionsDescriptor using the UUID attached to the PermissionsDescriptor. Hence, when dependency manager works with PermissionsDescriptor, it only knows about UUID and doesn't know anything about tableUUID/routineUUID and hashCode & equals method need to be able to handle that." Maybe my question is, at what point in time during the execution of a REVOKE do you end up with a PermissionsDescriptor that only has been populated with its object UUID, and what permission does the PermissionsDescriptor represent? I think I'm still a little in the dark as to the overall problem this patch is addressing, rather than the specific of a cache lookup not working.
          Hide
          Daniel John Debrunner added a comment -

          Mamta & I had a quick IM discussion to align our understanding of the issue:

          I was confused as to why in a simple REVOKE statement the DependencyManager (DM) was trying to fetch the PermissionsDescriptor for the item being revoked during the invalidation of said item. Ie. The call to DM.invalidateFor() takes the PermissionsDescriptor as the provider so why would it need to load it again?

          Thanks to Mamta's patience with my questions we discovered that during the DM.invalidateFor() call, the DM loads the set of persistent dependencies for the Provider (in this case the revoked permission) and then converts them to an in-memory form. In this conversion to an in-memory form the DM needs a Provider object to link it to the Dependent and loads the Dependent's Provider each time from the DataDictionary (DD) through its UUID. This is the load by UUID from the DD that was causing Mamta's code to have problems. The issue with this code is that the Provider object is the same for all the Dependents (since the list is derived from those objects dependent on a singel Provider) thus loading it multple times is inefficient, and it's the same Provider that was passed into invalidateFor() , thus no-need to load it from the DD at all.

          To verify this was indeed the case, I created a table T and created three views against the table. When a DROP TABLE T was executed I did see three loads of the TableDescriptor for T and it matched the TTableDescriptor passed into DM.invalidateFor.

          Thus what is really happening is that Mamta has un-earthed some inefficient behaviour in the DM implementation. I'll enter a separate bug for this and I have some code to address it, I need to run complete tests first. I'm slowed down by the fact it's so hot at home (heatwave & no a/c) I can't leave my laptop on too long otherwise it gets really hot.

          There may still be an issue with the permissions cache, I couldn't find any thing in the GRANT/REVOKE design spec or the code that described its use and design.

          Show
          Daniel John Debrunner added a comment - Mamta & I had a quick IM discussion to align our understanding of the issue: I was confused as to why in a simple REVOKE statement the DependencyManager (DM) was trying to fetch the PermissionsDescriptor for the item being revoked during the invalidation of said item. Ie. The call to DM.invalidateFor() takes the PermissionsDescriptor as the provider so why would it need to load it again? Thanks to Mamta's patience with my questions we discovered that during the DM.invalidateFor() call, the DM loads the set of persistent dependencies for the Provider (in this case the revoked permission) and then converts them to an in-memory form. In this conversion to an in-memory form the DM needs a Provider object to link it to the Dependent and loads the Dependent's Provider each time from the DataDictionary (DD) through its UUID. This is the load by UUID from the DD that was causing Mamta's code to have problems. The issue with this code is that the Provider object is the same for all the Dependents (since the list is derived from those objects dependent on a singel Provider) thus loading it multple times is inefficient, and it's the same Provider that was passed into invalidateFor() , thus no-need to load it from the DD at all. To verify this was indeed the case, I created a table T and created three views against the table. When a DROP TABLE T was executed I did see three loads of the TableDescriptor for T and it matched the TTableDescriptor passed into DM.invalidateFor. Thus what is really happening is that Mamta has un-earthed some inefficient behaviour in the DM implementation. I'll enter a separate bug for this and I have some code to address it, I need to run complete tests first. I'm slowed down by the fact it's so hot at home (heatwave & no a/c) I can't leave my laptop on too long otherwise it gets really hot. There may still be an issue with the permissions cache, I couldn't find any thing in the GRANT/REVOKE design spec or the code that described its use and design.
          Hide
          Mamta A. Satoor added a comment -

          I am attaching a patch (DERBY1539V2diffDropTriggerOnRevoke.txt) which supports basic revoke functionality for triggers. If revoke statement finds a trigger dependent on the table/column/routine on which privilege is being revoked, the trigger will be dropped automatically.

          I need to further fine tune this functionality with a subsequent patch, so that trigger will get dropped only if it depends on the particular privilege TYPE that is being revoked. For eg, a trigger might just need a SELECT privilege on a table(and doesn't depend on other kind of privileges on that table) but with this patch, the trigger will drop itself even when INSERT privilege is revoked on that same table.

          svn stat -q o/p is attached as DERBY1539V2statDropTriggerOnRevoke.txt

          Following changes are included in this patch
          1)Add a new invalidation action called REVOKE_PRIVILEGE in BasicDependencyManager and DependencyManager
          2)Had to make DropTriggerConstantAction.java and one of it's methods, dropTriggerDescriptor, public. This was required so that
          TriggerDescriptor can call DropTriggerConstantAction.dropTriggerDescriptor when it gets a revoke privilege for one of the privileges that is required by the TriggerDescriptor. DropTriggerConstantAction and TriggerDescriptor are not in the same package.
          3)DataDictionaryImpl - send invalidation messages when a revoke privilege is requested only if that privilege was really granted to that user. Currently, a user can revoke a privilege which was never granted. Derby will treat it as a no-op because there is nothing to revoke. That is why I moved the if condition in the patch to it's new location. With this change, invalidation will get sent only on the revoke privilege which revokes a granted privilege.
          4)For now, let ConstraintDescriptor and ViewDescriptor ignore revoke action. Will get to them in subsequent patch.
          5)TriggerDescriptor.java will drop the trigger if it receives the revoke action. This needs to be refined more in subsequent patch becasue with the current patch, the trigger will drop itself for any kind of privilege type revoke on a table. For eg, a trigger
          might just need a SELECT privilege on a table but the trigger will drop itself when INSERT privilege is revoked on that same table.
          6)Added more tests into lang/grantRevokeDDL.sql
          1) +ve test case
          mamta1
          create a table t11TriggerRevokeTest
          grant trigger on t11TriggerRevokeTest to mamta2
          mamta2
          create a trigger on t11TriggerRevokeTest
          mamta1
          do some dmls
          revoke trigger on t11TriggerRevokeTest from mamta2 – this will drop the dependent trigger
          mamta2
          attempt to recreate trigger on t11TriggerRevokeTest will fail.
          mamta1
          grant trigger privilege on t11TriggerRevokeTest
          mamta2
          recreates the trigger
          mamta1
          drop t11TriggerRevokeTest – drops dependent trigger
          2) -ve test case
          mamta1
          create a table t11TriggerRevokeTest
          grant select on t11TriggerRevokeTest to mamta2
          grant trigger on t11TriggerRevokeTest to mamta2
          mamta2
          create a trigger on t11TriggerRevokeTest
          mamta1
          do some dmls
          – following will drop the trigger eventhough trigger does not require SELECT privilege
          – This is incorrect. Will fix this in the next patch
          revoke SELECT on t11TriggerRevokeTest from mamta2
          mamta2
          attempt to recreate trigger on t11TriggerRevokeTest will pass because TRIGGER privilege is still in place.

          Show
          Mamta A. Satoor added a comment - I am attaching a patch (DERBY1539V2diffDropTriggerOnRevoke.txt) which supports basic revoke functionality for triggers. If revoke statement finds a trigger dependent on the table/column/routine on which privilege is being revoked, the trigger will be dropped automatically. I need to further fine tune this functionality with a subsequent patch, so that trigger will get dropped only if it depends on the particular privilege TYPE that is being revoked. For eg, a trigger might just need a SELECT privilege on a table(and doesn't depend on other kind of privileges on that table) but with this patch, the trigger will drop itself even when INSERT privilege is revoked on that same table. svn stat -q o/p is attached as DERBY1539V2statDropTriggerOnRevoke.txt Following changes are included in this patch 1)Add a new invalidation action called REVOKE_PRIVILEGE in BasicDependencyManager and DependencyManager 2)Had to make DropTriggerConstantAction.java and one of it's methods, dropTriggerDescriptor, public. This was required so that TriggerDescriptor can call DropTriggerConstantAction.dropTriggerDescriptor when it gets a revoke privilege for one of the privileges that is required by the TriggerDescriptor. DropTriggerConstantAction and TriggerDescriptor are not in the same package. 3)DataDictionaryImpl - send invalidation messages when a revoke privilege is requested only if that privilege was really granted to that user. Currently, a user can revoke a privilege which was never granted. Derby will treat it as a no-op because there is nothing to revoke. That is why I moved the if condition in the patch to it's new location. With this change, invalidation will get sent only on the revoke privilege which revokes a granted privilege. 4)For now, let ConstraintDescriptor and ViewDescriptor ignore revoke action. Will get to them in subsequent patch. 5)TriggerDescriptor.java will drop the trigger if it receives the revoke action. This needs to be refined more in subsequent patch becasue with the current patch, the trigger will drop itself for any kind of privilege type revoke on a table. For eg, a trigger might just need a SELECT privilege on a table but the trigger will drop itself when INSERT privilege is revoked on that same table. 6)Added more tests into lang/grantRevokeDDL.sql 1) +ve test case mamta1 create a table t11TriggerRevokeTest grant trigger on t11TriggerRevokeTest to mamta2 mamta2 create a trigger on t11TriggerRevokeTest mamta1 do some dmls revoke trigger on t11TriggerRevokeTest from mamta2 – this will drop the dependent trigger mamta2 attempt to recreate trigger on t11TriggerRevokeTest will fail. mamta1 grant trigger privilege on t11TriggerRevokeTest mamta2 recreates the trigger mamta1 drop t11TriggerRevokeTest – drops dependent trigger 2) -ve test case mamta1 create a table t11TriggerRevokeTest grant select on t11TriggerRevokeTest to mamta2 grant trigger on t11TriggerRevokeTest to mamta2 mamta2 create a trigger on t11TriggerRevokeTest mamta1 do some dmls – following will drop the trigger eventhough trigger does not require SELECT privilege – This is incorrect. Will fix this in the next patch revoke SELECT on t11TriggerRevokeTest from mamta2 mamta2 attempt to recreate trigger on t11TriggerRevokeTest will pass because TRIGGER privilege is still in place.
          Hide
          Mamta A. Satoor added a comment -

          BTW, the patch DERBY1539V1hashCodeEqualsDiff.txt should not be applied. Dan has checked in a fix for dependency manager so that we do not recreate the Provider object and hence there is no need for DERBY1539V1hashCodeEqualsDiff.txt. Dan's changes went in as part of DERBY-1581 (Revision: 425479)

          Show
          Mamta A. Satoor added a comment - BTW, the patch DERBY1539V1hashCodeEqualsDiff.txt should not be applied. Dan has checked in a fix for dependency manager so that we do not recreate the Provider object and hence there is no need for DERBY1539V1hashCodeEqualsDiff.txt. Dan's changes went in as part of DERBY-1581 (Revision: 425479)
          Hide
          Mamta A. Satoor added a comment -

          Forgot to mention, I ran derbyall with DERBY1539V2diffDropTriggerOnRevoke.txt on Windows XP with Sun jdk1.4 and there were no new failures.

          Show
          Mamta A. Satoor added a comment - Forgot to mention, I ran derbyall with DERBY1539V2diffDropTriggerOnRevoke.txt on Windows XP with Sun jdk1.4 and there were no new failures.
          Hide
          Daniel John Debrunner added a comment -

          The code in the patch seems generally ok, though it's not clear why you stopped resetting the UUID here and with changes below may still be required:

          //grant/revoke privilege didn't change anything and hence just

          • //return after resetting the uuid in the permission descriptor
          • perm.setUUID(null);
            + //return
            return;

          As an aside, that diff confused me for a while, I thought you were adding a commented out return statement, eventually I figured out it was a modified comment

          However two issues:

          1) the call to invalidateFor() in the DataDictionaryImpl is not correct, it needs to be moved higher, to the revoke constanct action that is driving the REVOKE. Most of the other existing calls to invalidateFor() are from the constant actions, and none are from within the data dictionary. Think of the data dictionary as the glue code between objects and the stored form, the system catalogs. Thus if one had a pure in-memory data dictionary one wouldn't want to duplicate the logic relating to revoking objects in it, much better to have it in a single place at the higher level.

          2) The actual drop of the trigger descriptor needs to be in the makeInvalid call, not the prepareToInvalidate call. See the earlier discussion on derby-dev.
          [As another aside, if code has an existing switch statement you should add to the switch, not ad dan extra if statement, like your code in prepareToInvalidate]

          Show
          Daniel John Debrunner added a comment - The code in the patch seems generally ok, though it's not clear why you stopped resetting the UUID here and with changes below may still be required: //grant/revoke privilege didn't change anything and hence just //return after resetting the uuid in the permission descriptor perm.setUUID(null); + //return return; As an aside, that diff confused me for a while, I thought you were adding a commented out return statement, eventually I figured out it was a modified comment However two issues: 1) the call to invalidateFor() in the DataDictionaryImpl is not correct, it needs to be moved higher, to the revoke constanct action that is driving the REVOKE. Most of the other existing calls to invalidateFor() are from the constant actions, and none are from within the data dictionary. Think of the data dictionary as the glue code between objects and the stored form, the system catalogs. Thus if one had a pure in-memory data dictionary one wouldn't want to duplicate the logic relating to revoking objects in it, much better to have it in a single place at the higher level. 2) The actual drop of the trigger descriptor needs to be in the makeInvalid call, not the prepareToInvalidate call. See the earlier discussion on derby-dev. [As another aside, if code has an existing switch statement you should add to the switch, not ad dan extra if statement, like your code in prepareToInvalidate]
          Hide
          Mamta A. Satoor added a comment -

          Dan, I have addressed your concerns in the patch DERBY1539V3diffDropTriggerOnRevoke.txt

          1)The invalidation messages are now sent by the caller rather than the method DataDictionary.addRemovePermissionsDescriptor. DataDictionary.addRemovePermissionsDescriptor will return true if it has made changes to system tables such that the dependents need to be notified.

          2)I have moved the drop trigger code into makeInvalid call.

          3)Removed the if condition and used the existing switch.

          The grant revoke tests passed with these changes.

          Can someone please review/commit this patch? thanks

          Show
          Mamta A. Satoor added a comment - Dan, I have addressed your concerns in the patch DERBY1539V3diffDropTriggerOnRevoke.txt 1)The invalidation messages are now sent by the caller rather than the method DataDictionary.addRemovePermissionsDescriptor. DataDictionary.addRemovePermissionsDescriptor will return true if it has made changes to system tables such that the dependents need to be notified. 2)I have moved the drop trigger code into makeInvalid call. 3)Removed the if condition and used the existing switch. The grant revoke tests passed with these changes. Can someone please review/commit this patch? thanks
          Hide
          Daniel John Debrunner added a comment -

          Mamta wrote:
          2)I have moved the drop trigger code into makeInvalid call.

          The v3 patch still has the drop trigger code in TriggerDescriptor.prepareToInvalidate

          Show
          Daniel John Debrunner added a comment - Mamta wrote: 2)I have moved the drop trigger code into makeInvalid call. The v3 patch still has the drop trigger code in TriggerDescriptor.prepareToInvalidate
          Hide
          Mamta A. Satoor added a comment -

          Sorry about the mix up. This is what I get for staying up late.

          I have attached the patch as DERBY1539V4diffDropTriggerOnRevoke.txt and hopefully followed through all my claims. It is same as the earlier patch except that trigger drop has been moved into makeInvalid method. I ran the grant revoke tests and they ran fine.

          Show
          Mamta A. Satoor added a comment - Sorry about the mix up. This is what I get for staying up late. I have attached the patch as DERBY1539V4diffDropTriggerOnRevoke.txt and hopefully followed through all my claims. It is same as the earlier patch except that trigger drop has been moved into makeInvalid method. I ran the grant revoke tests and they ran fine.
          Hide
          Daniel John Debrunner added a comment -

          Patch DERBY1539V4diffDropTriggerOnRevoke.txt Committed revision 425836

          Show
          Daniel John Debrunner added a comment - Patch DERBY1539V4diffDropTriggerOnRevoke.txt Committed revision 425836
          Hide
          Mamta A. Satoor added a comment -

          Recently, I had submitted a patch(DERBY1539V3diffDropTriggerOnRevoke.txt) for triggers which will drop the triggers if a revoke privilege is issued on a table/routine used by the trigger. That patch dropped the trigger even if the trigger didn't depend on the permission type/column on the table. And the patch dropped the trigger if the trigger depended on the routine.

          eg for current behavior on revoke table level privilege
          mamta1
          create table t1(c11 int, c12 int);
          grant select, update, trigger on t1 to mamta2
          mamta2
          create a trigger on mamta1.t1 with action as select * from some other table
          – notice that the trigger object above depends only on the trigger privilege on mamta1.t1
          mamta1
          revoke select on t1 from mamta2
          – this revoke ends up dropping the trigger even though trigger doesnot rely on select permission

          eg for current behavior on revoke column level privilege
          mamta1
          create table t1(c11 int, c12 int);
          grant trigger on t1 to mamta2
          create table t2(c21 int, c22 int);
          grant select(c21, c22) on t2 to mamta2
          mamta2
          create a trigger on mamta1.t1 with action as select c21 from mamta1.t2
          – notice that the trigger object above depends only on the trigger privilege on mamta1.t1, and
          – select privilege on mamta1.t2.c21
          mamta1
          revoke select(c22) on t2 from mamta2
          – this revoke ends up dropping the trigger even though trigger doesnot rely on select
          – permission on column c22 of table t2

          eg for current behavior on revoke column level privilege
          mamta1
          create function f1
          grant execute on f1 to mamta2
          mamta2
          create table t1
          create trigger on t1 with action that executes mamta1.f1
          mamta1
          revoke execute on f1 from mamta2 RESTRICT
          – this revoke ends up dropping the trigger even though revoke execute is supposed to have
          – RESTRICT behavior, which means that
          – if there are dependent objects, then revoke execute should fail. Couldn't implement this in earlier – patch because dependents didn't know what kind of revoke was issued. All they knew was revoke
          – issued against one of the objects that the dependent relied on.

          With the idea of working in incremental steps, I submitted the earlier patch as the first step towards implementing revoke privilege. As the next step, I am attaching another patch (DERBY1539V4diffDropTriggerOnRevokeRequiredPrivilege.txt ), which will fix the problem mentioned above in the egs. This patch currently only deals with triggers. The next steps are to implement similar behavior for views and constraints. The svn stat -q o/p for this patch is attached as DERBY1539V4statDropTriggerOnRevokeRequiredPrivilege.txt. Note that I have added a new file in this patch.

          The reason for implementing REVOKE EXECUTE ... RESTRICT in this patch is that prior to this patch, there was
          no way of knowing what kind of revoke privilege is issued by the user and hence even on revoke execute, I was dropping the dependent objects. With this patch, now we know what kind of revoke privilege has been issued and when the dependent gets revoke execute action, it can now throw an exception.

          Grant revoke tests have run fine with this patch. I fired derbyall suite couple hrs back on my Windows XP machine with Sun'd jdk1.4 and no errors so far.

          More information on the current patch's implementation details is as follows.
          1)BasicDependencyManager, TriggerDescriptor
          SYSTABLEPERMS has one row per table, grantee, grantor. That row has various fields to indicate what type of permissions(insert, trigger, update etc) are available for that key. The row is also uniquely identified by a UUID. Currently, when an object is created and it needs a particular pemission type on a given table, grantee, grantor, the dependency manager(DM) only tracks the dependency using UUID and it doesn't keep track of the exact permission type required. Because of this, currently, any permission type that gets revoked on table, grantee, grantor, it ends up dropping the dependent object, whether or not the dependent object really needs that permission type.
          eg(copying the eg from the beginning of this comment
          mamta1
          create table t1(c11 int, c12 int);
          grant select, update, trigger on t1 to mamta2
          mamta2
          create a trigger on mamta1.t1 with action as select * from some other table
          – notice that the trigger object above depends only on the trigger privilege on mamta1.t1
          mamta1
          revoke select on t1 from mamta2
          – this revoke ends up dropping the trigger even though trigger doesnot rely on select permission

          The problem also exists for column level permissions. SYSCOLPERMS has one row per table, grantee, grantor, a permission type and a bit map for columns on which that permission is granted. The row is also uniquely identified by a UUID. Now, an object might need a permission on only a subset of table columns and if a revoke is done later on columns that are not used by the object, then we should not drop the object. Currently, Derby tracks dependency on column level permissions using just the UUID and does not keep track of exact column subset required by the dependent object. Because of this, any column that gets revoked for a given UUID, DM drops all the dependents on that UUID, even if the dependent object does not care about the column being revoked.
          eg(copying the eg from the beginning of this comment
          mamta1
          create table t1(c11 int, c12 int);
          grant trigger on t1 to mamta2
          create table t2(c21 int, c22 int);
          grant select(c21, c22) on t2 to mamta2
          mamta2
          create a trigger on mamta1.t1 with action as select c21 from mamta1.t2
          – notice that the trigger object above depends only on the trigger privilege on mamta1.t1, and select privilege on mamta1.t2.c21
          mamta1
          revoke select(c22) on t2 from mamta2
          – this revoke ends up dropping the trigger even though trigger doesnot rely on select permission on column c22 of table t2

          To fix both these problems, I have enhanced the DM such that when an object is created which needs a particular permission type on a given permission table's UUID, DM tracks the object's dependency on the permission type and the row in SYSTABLEPERM identified by it's UUID. Similarly, when an object is created which needs a particular permission type on a subset of table's column, the object's dependnecy is tracked on the column subset and the row in SYSCOLPERMS identified by it's UUID. Both of these solutions follow the existing model that we currently have to track view's dependency on a subset of a table's columns.

          I have implemented this kind of dependency for SYSTABLEPERMS by adding a new class called DDPrivilegeTypeDependableFinder.java which saves the permission type part of the dependency tracking for an object. For SYSCOLPERMS, I implemented this by modifying existing class DDColumnDependableFinder.java. This class is currently used for tracking view's dependency on subset of a table's columns. Now, this class will also get used to track dependency on a subset of columns for column level privileges.

          The changes so far explained covers what happens when DM saves the dependencies. Later on, when a provider wants to notify it's dependents about an action, DM needs to use all the information that it collected while saving the dependencies.

          To be specific, later, when a permission is revoked, the DM needs to build a list of dependents that rely on that permission. For each of the dependents, DM needs to find the exact permission type or column subset required by that dependent. That information then should be saved in the provider object and we should put the provider+dependent object pair into the list of dependents. Once the list building is finished, each of the dependent objects will get the revoke invalidation action. The dependent object will check the provider object to see if the permission type or column subset being revoked is one of the things that it depends on. If yes, then it will drop itself. If not, then it will ignore the invalidation action.

          In order to implement this, amon other changes, I had to modify the code recently checked in by Dan for improving DM performance. Dan made changes to DM so that we do not recreate the provider object when building the dependents list for the provider because we already have access to the provider object. With my changes, while building the dependents list, if the provider is TablePermsDescriptor or ColPermsDescriptor, then for every dependent, I save into the provider object exactly what kind of permission type/column subset the dependent depends on. And I put the dependent and this modified Provider pair into the dependent list.

          With this change in dependency system, subsequently, when a permission type is revoked, the TablePermsDescriptor(Provider) will have the permission type being revoked and the permission type required by the dependent object. When the dependent object will receive the REVOKE action at table level, it will check the TablePermsDescriptor to see if the permission type being revoked is one of the permissions required by it, and if yes, then the dependent object will drop itself. Similar thing will happen for ColPermsDescriptor when it is the Provider of an invalidation action.

          2)CoreDDFinderClassInfo, DDPrivilegeTypeDependableFinder, DDColumnDependableFinder -
          We need a special finder class for COLUMNS_PERMISSION_FINDER_V01_ID because along with the row in the SYSCOLPERMS, we also need to track the exact column list required for a given dependent object. This is same as what Derby currently does for views, where a view might require only a subset of columns in a table. And hence, when tracking the view dependency, we need to know not just the SYSTABLES row but also the subset of columns required by view. Since similar scheme is required for COLUMNS_PERMISSION_FINDER_V01_ID, I have used existing DDColumnDependableFinder with some modifications to support additional format id.

          For similar reasons, We need special finder class for TABLE_PERMISSION_FINDER_V01_ID because along with the row in the SYSTABLEPERMS, we also need to track the exact privilege type required on a table for a given dependent object. Unlike COLUMNS_PERMISSION_FINDER_V01_ID, where I could use the existing class, DDColumnDependableFinder, I had to add a new dependable finder class called DDPrivilegeTypeDependableFinder to track exact required privilege type for a given row in SYSTABLEPERMS.

          3)SPSDescriptor
          For now, I am having SPSDescriptor ignore all the revoke invalidation actions. May need some work here when working on query plan invalidation for revoke privilege.

          4)ViewDescriptor, ConstraintDescriptor -
          For now, ignore all the revoke invalidation actions(except REVOKE_EXECUTE_PRIVILEGE). Derby supports only RESTRICT form of revoke execute and that means that if there are any dependent objects on execute permission on routine, revoke execute on that routine should fail. That is why, I have ViewDescriptor, ConstraintDescriptor catch REVOKE_EXECUTE_PRIVILEGE and throw exception. As for all the other revoke invalidation actions, I Will get to them in subsequent patch for ViewDescriptor, ConstraintDescriptor.

          5)PermissionsDescriptor, RoutinePermsDescriptor
          Have each of the PermissionsDescriptor send appropriate revoke invalidation action. For instance, if SELECT privilege is being revoked, then send REVOKE_SELECT_PRIVILEGE and so on and so forth. The dependent objects can take desired action depending on the type of privilege being revoked. In addition, error messages will be more specific since for instance, rather than saying revoke privilege failed, they can say revoke execute privilege failed.

          6)RoutinePermsDescriptor, TablePermsDescriptor and ColPermDescriptor
          Changed getObjectName method from
          return "Routine Privilege on " + routineName;
          to
          return routineName;
          This change in the method makes the error method from revoke execute more readable. With the original method, if the user tried to revoke the execute permission from the routine when there were dependent objects relying on that permission, the error message would be
          ERROR X0Y25: Operation 'REVOKE EXECUTE PRIVILEGE' cannot be performed on object 'Routine Privilege on SELECTFROMSPECIFICSCHEMA' because TRIGGER 'TR31T31' is dependent on that object.

          With the change in the method, user would get following message
          ERROR X0Y25: Operation 'REVOKE EXECUTE PRIVILEGE' cannot be performed on object 'SELECTFROMSPECIFICSCHEMA' because TRIGGER 'TR31T31' is dependent on that object.

          7)DependencyManager - describes all the invalidation actions for revoke.

          8)TablePermsDescriptor and ColPermDescriptor have changes to record speicifc privilege requirement of the dependent. These getter, setter and resetter methods are all called by the dependency manager to track object's dependency on specific privilege type and subset of table's columns.

          9)StatementTablePermission, StatementColumnPermission -
          During the compile phase of a view, trigger, constraint, we collect what privileges are required by these objects. The information about the required privileges, privilege types, column list needs to be saved by dependency manager in the execute phase. The change to these classes are for collecting required information for dependency manager so that it can save the specific privilege type/column subset requirements in dependency system.

          10)RoutinePrivilegeInfo, TablePrivilegeInfo
          Call methods on the PermissionDescriptors so that they can send specific revoke invalidation actions

          9)New Tests
          1)revoke execute should fail if there are dependent objects on it
          2)revoke will drop a trigger only if trigger depends on that specific privilege type.
          3)create trigger which depends on the privileges on objects from different schemas. Revoke on any one those objects should drop the trigger.
          4)create trigger which depends on both table and column level privileges. The trigger should get dropped only if the privileges gets revoked on specific columns required by the trigger or if privileges gets revoked on specific privilege type required by the trigger on a table. If not, then trigger should stay untouched.
          3)I couldn't add any testing for trigger action as update on tables owned by a different schema. This is because Derby runs into a NPE. The Jira entry for this specific problem is DERBY-1583. Once that bug is fixed, we should add tests like
          set connection mamta3
          create trigger tr11t11 after insert on mamta1.t11TriggerRevokeTest for each statement mode db2sql
          update mamta2.t21TriggerRevokeTest set c212 = 99;
          and then revoke update privileges on mamta2.t21TriggerRevokeTest from mamta3.

          I will appreciate if someone can review, commit this patch for me.

          Show
          Mamta A. Satoor added a comment - Recently, I had submitted a patch(DERBY1539V3diffDropTriggerOnRevoke.txt) for triggers which will drop the triggers if a revoke privilege is issued on a table/routine used by the trigger. That patch dropped the trigger even if the trigger didn't depend on the permission type/column on the table. And the patch dropped the trigger if the trigger depended on the routine. eg for current behavior on revoke table level privilege mamta1 create table t1(c11 int, c12 int); grant select, update, trigger on t1 to mamta2 mamta2 create a trigger on mamta1.t1 with action as select * from some other table – notice that the trigger object above depends only on the trigger privilege on mamta1.t1 mamta1 revoke select on t1 from mamta2 – this revoke ends up dropping the trigger even though trigger doesnot rely on select permission eg for current behavior on revoke column level privilege mamta1 create table t1(c11 int, c12 int); grant trigger on t1 to mamta2 create table t2(c21 int, c22 int); grant select(c21, c22) on t2 to mamta2 mamta2 create a trigger on mamta1.t1 with action as select c21 from mamta1.t2 – notice that the trigger object above depends only on the trigger privilege on mamta1.t1, and – select privilege on mamta1.t2.c21 mamta1 revoke select(c22) on t2 from mamta2 – this revoke ends up dropping the trigger even though trigger doesnot rely on select – permission on column c22 of table t2 eg for current behavior on revoke column level privilege mamta1 create function f1 grant execute on f1 to mamta2 mamta2 create table t1 create trigger on t1 with action that executes mamta1.f1 mamta1 revoke execute on f1 from mamta2 RESTRICT – this revoke ends up dropping the trigger even though revoke execute is supposed to have – RESTRICT behavior, which means that – if there are dependent objects, then revoke execute should fail. Couldn't implement this in earlier – patch because dependents didn't know what kind of revoke was issued. All they knew was revoke – issued against one of the objects that the dependent relied on. With the idea of working in incremental steps, I submitted the earlier patch as the first step towards implementing revoke privilege. As the next step, I am attaching another patch (DERBY1539V4diffDropTriggerOnRevokeRequiredPrivilege.txt ), which will fix the problem mentioned above in the egs. This patch currently only deals with triggers. The next steps are to implement similar behavior for views and constraints. The svn stat -q o/p for this patch is attached as DERBY1539V4statDropTriggerOnRevokeRequiredPrivilege.txt. Note that I have added a new file in this patch. The reason for implementing REVOKE EXECUTE ... RESTRICT in this patch is that prior to this patch, there was no way of knowing what kind of revoke privilege is issued by the user and hence even on revoke execute, I was dropping the dependent objects. With this patch, now we know what kind of revoke privilege has been issued and when the dependent gets revoke execute action, it can now throw an exception. Grant revoke tests have run fine with this patch. I fired derbyall suite couple hrs back on my Windows XP machine with Sun'd jdk1.4 and no errors so far. More information on the current patch's implementation details is as follows. 1)BasicDependencyManager, TriggerDescriptor SYSTABLEPERMS has one row per table, grantee, grantor. That row has various fields to indicate what type of permissions(insert, trigger, update etc) are available for that key. The row is also uniquely identified by a UUID. Currently, when an object is created and it needs a particular pemission type on a given table, grantee, grantor, the dependency manager(DM) only tracks the dependency using UUID and it doesn't keep track of the exact permission type required. Because of this, currently, any permission type that gets revoked on table, grantee, grantor, it ends up dropping the dependent object, whether or not the dependent object really needs that permission type. eg(copying the eg from the beginning of this comment mamta1 create table t1(c11 int, c12 int); grant select, update, trigger on t1 to mamta2 mamta2 create a trigger on mamta1.t1 with action as select * from some other table – notice that the trigger object above depends only on the trigger privilege on mamta1.t1 mamta1 revoke select on t1 from mamta2 – this revoke ends up dropping the trigger even though trigger doesnot rely on select permission The problem also exists for column level permissions. SYSCOLPERMS has one row per table, grantee, grantor, a permission type and a bit map for columns on which that permission is granted. The row is also uniquely identified by a UUID. Now, an object might need a permission on only a subset of table columns and if a revoke is done later on columns that are not used by the object, then we should not drop the object. Currently, Derby tracks dependency on column level permissions using just the UUID and does not keep track of exact column subset required by the dependent object. Because of this, any column that gets revoked for a given UUID, DM drops all the dependents on that UUID, even if the dependent object does not care about the column being revoked. eg(copying the eg from the beginning of this comment mamta1 create table t1(c11 int, c12 int); grant trigger on t1 to mamta2 create table t2(c21 int, c22 int); grant select(c21, c22) on t2 to mamta2 mamta2 create a trigger on mamta1.t1 with action as select c21 from mamta1.t2 – notice that the trigger object above depends only on the trigger privilege on mamta1.t1, and select privilege on mamta1.t2.c21 mamta1 revoke select(c22) on t2 from mamta2 – this revoke ends up dropping the trigger even though trigger doesnot rely on select permission on column c22 of table t2 To fix both these problems, I have enhanced the DM such that when an object is created which needs a particular permission type on a given permission table's UUID, DM tracks the object's dependency on the permission type and the row in SYSTABLEPERM identified by it's UUID. Similarly, when an object is created which needs a particular permission type on a subset of table's column, the object's dependnecy is tracked on the column subset and the row in SYSCOLPERMS identified by it's UUID. Both of these solutions follow the existing model that we currently have to track view's dependency on a subset of a table's columns. I have implemented this kind of dependency for SYSTABLEPERMS by adding a new class called DDPrivilegeTypeDependableFinder.java which saves the permission type part of the dependency tracking for an object. For SYSCOLPERMS, I implemented this by modifying existing class DDColumnDependableFinder.java. This class is currently used for tracking view's dependency on subset of a table's columns. Now, this class will also get used to track dependency on a subset of columns for column level privileges. The changes so far explained covers what happens when DM saves the dependencies. Later on, when a provider wants to notify it's dependents about an action, DM needs to use all the information that it collected while saving the dependencies. To be specific, later, when a permission is revoked, the DM needs to build a list of dependents that rely on that permission. For each of the dependents, DM needs to find the exact permission type or column subset required by that dependent. That information then should be saved in the provider object and we should put the provider+dependent object pair into the list of dependents. Once the list building is finished, each of the dependent objects will get the revoke invalidation action. The dependent object will check the provider object to see if the permission type or column subset being revoked is one of the things that it depends on. If yes, then it will drop itself. If not, then it will ignore the invalidation action. In order to implement this, amon other changes, I had to modify the code recently checked in by Dan for improving DM performance. Dan made changes to DM so that we do not recreate the provider object when building the dependents list for the provider because we already have access to the provider object. With my changes, while building the dependents list, if the provider is TablePermsDescriptor or ColPermsDescriptor, then for every dependent, I save into the provider object exactly what kind of permission type/column subset the dependent depends on. And I put the dependent and this modified Provider pair into the dependent list. With this change in dependency system, subsequently, when a permission type is revoked, the TablePermsDescriptor(Provider) will have the permission type being revoked and the permission type required by the dependent object. When the dependent object will receive the REVOKE action at table level, it will check the TablePermsDescriptor to see if the permission type being revoked is one of the permissions required by it, and if yes, then the dependent object will drop itself. Similar thing will happen for ColPermsDescriptor when it is the Provider of an invalidation action. 2)CoreDDFinderClassInfo, DDPrivilegeTypeDependableFinder, DDColumnDependableFinder - We need a special finder class for COLUMNS_PERMISSION_FINDER_V01_ID because along with the row in the SYSCOLPERMS, we also need to track the exact column list required for a given dependent object. This is same as what Derby currently does for views, where a view might require only a subset of columns in a table. And hence, when tracking the view dependency, we need to know not just the SYSTABLES row but also the subset of columns required by view. Since similar scheme is required for COLUMNS_PERMISSION_FINDER_V01_ID, I have used existing DDColumnDependableFinder with some modifications to support additional format id. For similar reasons, We need special finder class for TABLE_PERMISSION_FINDER_V01_ID because along with the row in the SYSTABLEPERMS, we also need to track the exact privilege type required on a table for a given dependent object. Unlike COLUMNS_PERMISSION_FINDER_V01_ID, where I could use the existing class, DDColumnDependableFinder, I had to add a new dependable finder class called DDPrivilegeTypeDependableFinder to track exact required privilege type for a given row in SYSTABLEPERMS. 3)SPSDescriptor For now, I am having SPSDescriptor ignore all the revoke invalidation actions. May need some work here when working on query plan invalidation for revoke privilege. 4)ViewDescriptor, ConstraintDescriptor - For now, ignore all the revoke invalidation actions(except REVOKE_EXECUTE_PRIVILEGE). Derby supports only RESTRICT form of revoke execute and that means that if there are any dependent objects on execute permission on routine, revoke execute on that routine should fail. That is why, I have ViewDescriptor, ConstraintDescriptor catch REVOKE_EXECUTE_PRIVILEGE and throw exception. As for all the other revoke invalidation actions, I Will get to them in subsequent patch for ViewDescriptor, ConstraintDescriptor. 5)PermissionsDescriptor, RoutinePermsDescriptor Have each of the PermissionsDescriptor send appropriate revoke invalidation action. For instance, if SELECT privilege is being revoked, then send REVOKE_SELECT_PRIVILEGE and so on and so forth. The dependent objects can take desired action depending on the type of privilege being revoked. In addition, error messages will be more specific since for instance, rather than saying revoke privilege failed, they can say revoke execute privilege failed. 6)RoutinePermsDescriptor, TablePermsDescriptor and ColPermDescriptor Changed getObjectName method from return "Routine Privilege on " + routineName; to return routineName; This change in the method makes the error method from revoke execute more readable. With the original method, if the user tried to revoke the execute permission from the routine when there were dependent objects relying on that permission, the error message would be ERROR X0Y25: Operation 'REVOKE EXECUTE PRIVILEGE' cannot be performed on object 'Routine Privilege on SELECTFROMSPECIFICSCHEMA' because TRIGGER 'TR31T31' is dependent on that object. With the change in the method, user would get following message ERROR X0Y25: Operation 'REVOKE EXECUTE PRIVILEGE' cannot be performed on object 'SELECTFROMSPECIFICSCHEMA' because TRIGGER 'TR31T31' is dependent on that object. 7)DependencyManager - describes all the invalidation actions for revoke. 8)TablePermsDescriptor and ColPermDescriptor have changes to record speicifc privilege requirement of the dependent. These getter, setter and resetter methods are all called by the dependency manager to track object's dependency on specific privilege type and subset of table's columns. 9)StatementTablePermission, StatementColumnPermission - During the compile phase of a view, trigger, constraint, we collect what privileges are required by these objects. The information about the required privileges, privilege types, column list needs to be saved by dependency manager in the execute phase. The change to these classes are for collecting required information for dependency manager so that it can save the specific privilege type/column subset requirements in dependency system. 10)RoutinePrivilegeInfo, TablePrivilegeInfo Call methods on the PermissionDescriptors so that they can send specific revoke invalidation actions 9)New Tests 1)revoke execute should fail if there are dependent objects on it 2)revoke will drop a trigger only if trigger depends on that specific privilege type. 3)create trigger which depends on the privileges on objects from different schemas. Revoke on any one those objects should drop the trigger. 4)create trigger which depends on both table and column level privileges. The trigger should get dropped only if the privileges gets revoked on specific columns required by the trigger or if privileges gets revoked on specific privilege type required by the trigger on a table. If not, then trigger should stay untouched. 3)I couldn't add any testing for trigger action as update on tables owned by a different schema. This is because Derby runs into a NPE. The Jira entry for this specific problem is DERBY-1583 . Once that bug is fixed, we should add tests like set connection mamta3 create trigger tr11t11 after insert on mamta1.t11TriggerRevokeTest for each statement mode db2sql update mamta2.t21TriggerRevokeTest set c212 = 99; and then revoke update privileges on mamta2.t21TriggerRevokeTest from mamta3. I will appreciate if someone can review, commit this patch for me.
          Hide
          Daniel John Debrunner added a comment -

          I'm looking at this patch and need to study it more, but I think having the dependency manager take specific actions based upon the type of provider or dependent is not a good approach. I know the code does this already for TableDescriptors (at least) and it's fair you copied that approach, but I'd like to think about if it can be avoided.

          Show
          Daniel John Debrunner added a comment - I'm looking at this patch and need to study it more, but I think having the dependency manager take specific actions based upon the type of provider or dependent is not a good approach. I know the code does this already for TableDescriptors (at least) and it's fair you copied that approach, but I'd like to think about if it can be avoided.
          Hide
          Daniel John Debrunner added a comment -

          I'm wondering if one could see the invalidation due to a revoke in a different light, more of a invalidation followed by a re-validate. Thus rather than having to carry around extra information about what exactly has changed, just have the trigger (dependent) receive notification that its privileges have changed in some way and now it needs to re-validate and check all of its permissions. Thus which privilege has dropped is no that important, just that fact that re-validation is required.

          I think a step like this is needed anyway if an object was dependent on a privilege granted to an individual that existed at create time but subsequently could be statisfied by the same privilege granted to PUBLIC.

          I assume that trigger re-compilation like this must work, say a trigger's action statement is defined as an INSERT, then that statement must need to be recompiled if an index is added or dropped from the table. Or with the various trigger re-compile bugs recently is trigger re-compilation not implemented at all?

          Show
          Daniel John Debrunner added a comment - I'm wondering if one could see the invalidation due to a revoke in a different light, more of a invalidation followed by a re-validate. Thus rather than having to carry around extra information about what exactly has changed, just have the trigger (dependent) receive notification that its privileges have changed in some way and now it needs to re-validate and check all of its permissions. Thus which privilege has dropped is no that important, just that fact that re-validation is required. I think a step like this is needed anyway if an object was dependent on a privilege granted to an individual that existed at create time but subsequently could be statisfied by the same privilege granted to PUBLIC. I assume that trigger re-compilation like this must work, say a trigger's action statement is defined as an INSERT, then that statement must need to be recompiled if an index is added or dropped from the table. Or with the various trigger re-compile bugs recently is trigger re-compilation not implemented at all?
          Hide
          Mamta A. Satoor added a comment -

          I am going to spend more time on your suggestion but from a quick look, I don't see any recompilation happening for triggers currently. Seems like triggers currently just ignore all the invalidations that come their way except for drop table/drop synonym etc. For these specific drop actions, triggers just raise an error that they are dependent on the object being dropped. But no recompilation seem to happen for any invalidation.

          Also, we would need recompilation mechanism for views and constraints too because they are also dependent on privileges and hence will be notified of revoke actions. Don't think there is any code for views and constraints to get recompiled. Will investigate into this more to be sure what happens if recompilation is required.

          Show
          Mamta A. Satoor added a comment - I am going to spend more time on your suggestion but from a quick look, I don't see any recompilation happening for triggers currently. Seems like triggers currently just ignore all the invalidations that come their way except for drop table/drop synonym etc. For these specific drop actions, triggers just raise an error that they are dependent on the object being dropped. But no recompilation seem to happen for any invalidation. Also, we would need recompilation mechanism for views and constraints too because they are also dependent on privileges and hence will be notified of revoke actions. Don't think there is any code for views and constraints to get recompiled. Will investigate into this more to be sure what happens if recompilation is required.
          Hide
          Mamta A. Satoor added a comment -

          Another solution I am thinking is have the ViewDescriptor, ConstraintDescriptor and TriggerDescriptor keep the information about what privileges they require. This can be done when these objects get loaded from the data dictionary. Then when they get the specific revoke actions like what my patch sends, the dependent objects will check their privilege requirement against what is being revoked in the PermissionDescriptor and then drop themselves if their required privilege is being revoked. So, rather than contaminate the dependency manager code with special cases, have the dependent descriptors aware of their privilege requirements. This seems to sound more logical. This will not solve the problem of having the dependent object depend on the next available privilege when one of it's current privilege is revoked but may be it can be tackled later.

          Let me know if anyone has any opinion about this approach. Otherwise, I am thinking I am going to start looking at this approach.

          Show
          Mamta A. Satoor added a comment - Another solution I am thinking is have the ViewDescriptor, ConstraintDescriptor and TriggerDescriptor keep the information about what privileges they require. This can be done when these objects get loaded from the data dictionary. Then when they get the specific revoke actions like what my patch sends, the dependent objects will check their privilege requirement against what is being revoked in the PermissionDescriptor and then drop themselves if their required privilege is being revoked. So, rather than contaminate the dependency manager code with special cases, have the dependent descriptors aware of their privilege requirements. This seems to sound more logical. This will not solve the problem of having the dependent object depend on the next available privilege when one of it's current privilege is revoked but may be it can be tackled later. Let me know if anyone has any opinion about this approach. Otherwise, I am thinking I am going to start looking at this approach.
          Hide
          Mamta A. Satoor added a comment -

          Thinking further about my proposed solution in the comment before this, I think it will be better for dependent descriptors to load their privilege requirements when they receive their first revoke invalidation action. This way, we don't unnecessarily have to load privilege requirements for all the triggers, constraints and view descriptors that get referenced in a database session. Especially, when the SQL Standard Authorization is not enabled, this list is not required at all. So, rather than loading the privilege requirements when the objects get loaded from the data dictionary, load the privilege requirements when the objects receive their first revoke invalidation action.

          This is all based on my thoughts in last hr or so and hence might have gottchas in them. If anyone catches anything in this solution, please let me know. In the mean time, I will continue to go this path.

          Show
          Mamta A. Satoor added a comment - Thinking further about my proposed solution in the comment before this, I think it will be better for dependent descriptors to load their privilege requirements when they receive their first revoke invalidation action. This way, we don't unnecessarily have to load privilege requirements for all the triggers, constraints and view descriptors that get referenced in a database session. Especially, when the SQL Standard Authorization is not enabled, this list is not required at all. So, rather than loading the privilege requirements when the objects get loaded from the data dictionary, load the privilege requirements when the objects receive their first revoke invalidation action. This is all based on my thoughts in last hr or so and hence might have gottchas in them. If anyone catches anything in this solution, please let me know. In the mean time, I will continue to go this path.
          Hide
          Daniel John Debrunner added a comment -

          On the trigger re-compile issue it seems there is a serious problem currently (regardless of grant/revoke issues) with triggers. This may need to be fixed before modifying the trigger -revoke interation any more.

          I wonder if you can make progress on the view & constraint dropping before completing the trigger? At least get them in to a similar state as triggers where a revoke wil drop the object, even if it drops it in too many situations.

          Show
          Daniel John Debrunner added a comment - On the trigger re-compile issue it seems there is a serious problem currently (regardless of grant/revoke issues) with triggers. This may need to be fixed before modifying the trigger -revoke interation any more. I wonder if you can make progress on the view & constraint dropping before completing the trigger? At least get them in to a similar state as triggers where a revoke wil drop the object, even if it drops it in too many situations.
          Hide
          Daniel John Debrunner added a comment -

          Cleaning the summary.

          Show
          Daniel John Debrunner added a comment - Cleaning the summary.
          Hide
          Yip Ng added a comment -

          The trigger recompilation problem has been resolved by DERBY-1621 and is already checked into the trunk. So this jira can continue to proceed.

          Show
          Yip Ng added a comment - The trigger recompilation problem has been resolved by DERBY-1621 and is already checked into the trunk. So this jira can continue to proceed.
          Hide
          Mamta A. Satoor added a comment -

          Closing this issue since I am finished with the work targeted for 10.2 release. The remaining work for this issue is getting tracked in DERBY-1782. I will link the 2 issues so we can keep track of the correlation between the 2 jira entries.

          Show
          Mamta A. Satoor added a comment - Closing this issue since I am finished with the work targeted for 10.2 release. The remaining work for this issue is getting tracked in DERBY-1782 . I will link the 2 issues so we can keep track of the correlation between the 2 jira entries.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development