|
Note that since the statement may be cached by the system and reuse by other users of different
connections, the suggested scheme of loading privilege(s) at compilation time may not eliminate the DD access in all cases if the system were to load the privilege info based on the auth id that compiles the statement. Yip, are you saying that we use the *user's* transaction to read table and column
permissions information from the data dictionary? I would have expected us to use some sort of internal "system" transaction for this kind of catalog fetch. Bryan, at compilation time, Derby uses an internal system nested transaction. After compilation is completed, this nested system
transaction is committed; therefore, the shared locks for system tables are released. However, at execution time of this jira case, the activation class is querying the data dictionary for permission since those descriptors are not in the permission cache, therefore, the shared locks are obtained for those system tables using the user transaction. I debugged into the code abit today, I don't think the system is fetching the permission descriptor(s)
from the system catalog at compilation time for the stated problem of this jira. During bind phase, the system just collects various permission(s) that are required to execute the select statement. It is at the execution time of the statement that the descriptors are fetched from the data dictionary. Attaching initial patch derby1716-trunk-diff01a.txt for
Hi Yip, thanks for the patch!
I read through the code and it looks good. Thanks for the clear comments in the addToPermissionsCache method and for the good tests. I noticed as part of studying does not clear the DataDictionary permissions cache. This seems wrong to me, and although it probably isn't *directly* related to this JIRA issue, I still wanted to bring it up since you've been studying this code, and get your opinion. Regarding your patch, I noticed you called it an "initial patch". Do you feel that this patch is ready for commit? Or do you think that there is additional work to do? Are other reviewers looking at this patch? Addendum to previous comment: the patch applies cleanly for me, and I
verified that, without the code change, the test case demonstrates the bug, and with the code change, the test case passes. I am +1 to commit, and I propose to commit this change if Yip feels that it is ready for commit. Hi Bryan, thanks for taking the time for reviewing the patch. For the clearCaches() part, it looks like the
permission cache handling are somewhat different from other caches. For instance, when a table gets dropped, its permission descriptor gets removed from the cache in its constant action(DROP TABLE) via DataDictionary's dropAllTableAndColPermDescriptors(). So they do get removed from the permission cache. I am trying another different approach so please do not commit the current patch just yet. I will post my findings soon. Thanks. unchecking patch available, contributer has asked for this patch to not be applied yet.
Attaching patch derby1716-trunk-diff02.txt for
After further analysis, I think the previous patch does not address all cases. In particular, unlike other descriptors, when privilege(s) get revoked from user, the statement is not subject to recompilation, so then we are back to square one since the previous patch attempts to bring in the permission descriptor(s) into the permission cache at compilation time to avoid reading from system tables at execution time. I believe the proper proposal fix is to use internal nested read-only transaction when the system is reading permission descriptors from the system tables. At a high level, a statement undergoes the following typical steps for it to get executed by the system: 1. Statement Compilation Phase a) Parse the statement b) Bind the statement and collects required permissions for it to be executed. c) Optimize the statement d) Generate the activation for the statement 2. Statement Execution Phase a) Check if the authoration id has the required privileges to execute the statement. b) Execute the statement The problem lies in permissions checking step at statement execution phase. Before a statement can be executed in SQL authorization mode, the authorization id's privileges needs to be check against the permission cache or if the privileges are not available in the cache, the system needs to read this metadata information from the system tables. But the system is using *user transaction* to do this, so the shared locks that got acquired by the user transaction may not get released immediately; therefore, leading to lock timeout when the grantor attempts to revoke the user's privilege. To resolve this issue, the system now will start an internal read-only nested transaction(same lock space as the parent transaction) to read permission related info from the system tables and release the shared locks as soon as the permissions check is completed before statement execution. This tackles the root of the stated problem. derbyall passes. Appreciate if someone can review the code changes. Thanks. Other interesting observations while going through the code: (1) In the current implementation, privileges collection actually ends at the time where the constant action is created and not at bind time. e.g.: DROP TABLE has schema permission collected at that time. (2) Permissions cache handling are different from other data dictionary caches. The other caches gets *ALL* cleared out either at startReading() or startWriting() time in Data Dictionary depending on the current cache mode. However, only the "affected" cached items in the permissions cache are removed and this logic is handled by the statements themselves and not the data dictionary. could you regenerate this patch, I am getting failures in applying the test portions of the patch -
grantRevokeDDL.sql has been a hot file. Committed this patch to the trunk.
m3_ibm142:20>svn commit Sending java\engine\org\apache\derby\impl\sql\conn\GenericAuthorizer.java Sending java\testing\org\apache\derbyTesting\functionTests\master\grantRevokeDDL.out Sending java\testing\org\apache\derbyTesting\functionTests\tests\lang\grantRevokeDDL.sql Sending java\testing\org\apache\derbyTesting\functionTests\tests\lang\grantRevokeDDL_app.properties Transmitting file data .... Committed revision 453935. Ported to 10.2 branch at subversion revision 480148.
updated the 10.2 j9_foundation canon with revision 486380.
This issue has been resolved for over a year with no further movement. Closing.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Major
Revoking select privilege from a user times out when that user still have a cursor open.
execution time (at the start of fillResultSet() of the activation class) since the grantee's privileges
are not in the permission cache yet. Thus, the shared locks will last till end of the transaction.
This is done once and those privileges that were loaded from the data dictionary(DD) will be stored
in the permission cache for use later so that the system does not need to load those info again
from the DD. Perhaps these privilege info can be loaded at compilation time instead as an improvement,
so that at execution time, privilege related metadata can be retrieved from the permission cache instead
of from the DD.
protected ResultSet fillResultSet() throws StandardException
{
((Activation) this).getLanguageConnectionContext().getAuthorizer().authorize( (Activation) this, 1 );
return getResultSetFactory().getScrollInsensitiveResultSet( <snip> );
}