|
[
Permlink
| « Hide
]
Ritika Maheshwari added a comment - 28/Mar/07 09:58 PM
here is a patch for this feature
Ritika Maheshwari made changes - 28/Mar/07 09:58 PM
Wow... that's a lot more DB2 locking knowledge than I've ever seen in one place. Neat.
Some comments on the patch: 1. How does the openjpa.hint.updateClause hint differ from the value of the forUpdate flag passed in to the DBDictionary call? It looks like the existing OpenJPA per-transaction read / write lock level configuration could be used instead. 2. Is openjpa.hint.isolationLevel really a hint, or more of a rule? Again, I have a hunch that maybe we could do something with the read / write lock levels, or maybe some other means of controlling isolation level. In any event, it seems like isolation level isn't really a hint, but rather is more of a rule. 3. You introduced a number of public boolean fields for determining what type of DB2 instance is being used. Based on code inspection, it looks like you expect that it should always be possible to automatically determine the type; maybe these should be private fields instead? We only have public fields in DBDictionaries for user-configurable settings. Also, since it looks like only one of the booleans can meaningfully be true, I'd rather see a single private db2ServerType field that will be set to one of several symbolic constant values. This will let you replace the if-else block with a switch block if you prefer that sort of thing, also. 1. The updateFlag is set to true when the JPA query is running in pessimistic mode.Where as with hint even when u are in optimistic mode you can get the update flags.This way user can use the standard JPA api setHint and get the lock clause.Where as in the other case they need to use OpenJPA specific api.Besides hint is per query basis.We also needed the hints for Static SQL and Named Queries.
2. I do not understand what u mean by rule versus hint.Isolation level hint tells us whther to add WITH RR or WITH RS.And again this is per query basis where as the isolation hint through the persistence.xml is to broad applying to all the entities.Then again we need the hints for NamedQuery and Static SQL. 3. Agreed
For the sake of discussion, I've attached an alternate patch that uses a new JDBCFetchPlan.setIsolationLevel() instead of a hint for isolation level, and uses JDBCFetchConfiguration.getReadLockLevel() to determine whether or not to do a SELECT ... FOR UPDATE.
If the read lock level is set to LockLevels.LEVEL_WRITE, then the FOR UPDATE is included; if the read lock level is set to LockLevels.LEVEL_READ, then no FOR UPDATE is used. If the read lock level is LockLevels.LEVEL_NONE, then the default behavior is used. (This is possibly not the best use of LEVEL_NONE -- I'm not sure if LEVEL_NONE means 'default' or something else. But for the purposes of demonstration, it seemed expedient to use it. Adding a new LEVEL_DEFAULT constant might make more sense.) Also, I directly reused the java.sql.Connection constants, which is possibly non-ideal; we might want to discuss making our own constants. Or not. So, in this model, if there were a test case for this stuff, I would have changed the test case to do: Query q = em.createQuery(...); JDBCFetchPlan plan = (JDBCFetchPlan) ((OpenJPAQuery) query).getFetchPlan(); plan.setIsolationLevel(Connection.TRANSACTION_SERIALIZABLE); plan.setReadLockMode(LockModeType.WRITE); // force a FOR UPDATE List l = q.getResultList(); Note also that this model allows the isolation level and read lock mode to be configured on the EM itself, for use in find() calls and relationship lookups, and as the default settings for the fetch plans of queries created from the EM. Finally, I replicated the logic in DB2Dictionary, but I noticed that sometimes the logic checked for "serializable" and sometimes it checked for "read-uncommitted". I preserved these checks, but was this intentional? I'm not all that clear about what the optimizations are, so just wanted to ensure that that wasn't a typo.
Patrick Linskey made changes - 04/Apr/07 11:49 PM
A use-case for the isolation level is to support different locking semantics for different classes and possibly for different queries. To use Patrick's patch would I need to use a different fetch plan before issuing a find or a query and then set it back after that method call? Or can I specify an isolation level in the FetchConfiguration per class?
In the hint-based approach, you could set the hints on a particular query.
In my approach, you could configure the FetchPlan for a particular query (get the Query's fetch plan), or for a particular entity manager (get the EM's fetch plan). To do different fetch plans for different queries, you'd just set the different query fetch plans differently. Query fetch plans are a copy-on-write semantic -- they start off just delegating through to the EM's fetch plan, and if you make changes, then they immediately make a copy of the EM's fetch plan, and remain isolated from the EM's fetch plan from then on out. If you wanted to do different isolation levels for different find() calls or different relationship traversals, in my impl, you'd need to set the EM's fetch plan, do the work, then set it back. In the hint-based impl, you would not be able to do any configuration for find() or relationship traversals. It's probably worth noting that everything in the FetchPlan is configurable via the Query.setHint() APIs. My earlier theoretical test case would become:
Query q = em.createQuery(...); q.setHint("openjpa.FetchPlan.IsolationLevel", Connection.TRANSACTION_SERIALIZABLE); q.setHint("openjpa.FetchPlan.ReadLockMode", LockModeType.WRITE); List l = q.getResultList(); In other words, if we use the patch that I attached (or something like it), we will end up with strong typing, API alignment with similar concepts, and dynamic accessibility both in terms of Query.setHint() and @QueryHints annotations / query-hint XML. What is setting the isolation level this way actually doing? For anything other than DB2 right now, it looks like it doesn't do anything. And even for DB2, it's unclear to me exactly what the isolation level hint is doing, and why the information can't be gleaned from the global setting or the connection. It seems very misleading to me to have a setIsolationLevel API (or generic "IsolationLevel" hint) that doesn't actually change the connection's isolation level.
If we can determine that this API is actually useful for more than DB2, and if we can name it appropriately for what it actually does, then I like Patrick's proposal of a FetchPlan API. The fact that all FetchPlan properties can be expressed as hints should make everyone happy. But if this is just a DB2 thing, then we should rename the hint to have a DB2-specific name and be done with it IMO. Hopefully, this will be useful for applications where there are "hot" tables that require pessimistic locking even though the rest of the application does better with optimistic. Take the example of an Order/OrderLine where there are lots of updates of the Order and/or associated OrderLine. If several threads get the same Order, they will ultimately conflict and waste time. If the query to retrieve the Order is marked as pessimistic (WRITE) then these threads will serialize and all of their work will complete.
> Hopefully, this will be useful for applications where there are "hot" tables that require pessimistic locking even though the rest of the application does better with optimistic.
That's what our lock levels and lock APIs are for. I'm still not clear on what this is adding to the mix for most DBs. > > Hopefully, this will be useful for applications where there are "hot" tables
> > that require pessimistic locking even though the rest of the application > > does better with optimistic. > > That's what our lock levels and lock APIs are for. I'm still not clear on what this is > adding to the mix for most DBs. Not really -- the lock levels allow the user to configure how locking should happen, not what the isolation level should be for the locks. I don't know about what levels of support non-DB2 databases have for per-query isolation level configuration. Does anyone have any experience with this in other databases? Oh, and regardless, we should change the base DBDictionary to throw an exception if this FetchPlan setting is set but not serviceable. One thing that we should test: I'm not convinced that the lock level override in the DB2Dictionary code is necessary. It's possible that the LockManager will already take into account the current JDBCFetchConfiguration's lock level settings when specifying the forUpdate setting for the toSelect() clause. Some test cases will make it easy to figure out the answer to this question. I know that Oracle allows you to add a FOR UPDATE clause to a query, and this affects the results of that query. In Sun appserver CMP we use this to set exclusive locks on rows where we want pessimistic locking behavior just for certain tables.
> I know that Oracle allows you to add a FOR UPDATE clause to a query, and this affects the results of that query. In Sun appserver CMP we use this to set exclusive locks on rows where we want pessimistic locking behavior just for certain tables.
Again, this is exactly what our existing lock levels and APIs do. > > I know that Oracle allows you to add a FOR UPDATE clause
> > to a query, and this affects the results of that query. In Sun > > appserver CMP we use this to set exclusive locks on rows > > where we want pessimistic locking behavior just for certain tables. > > Again, this is exactly what our existing lock levels and APIs do. The current patches in this issue encompass two separate bits of functionality: 1. an update-override setting. (In the context of my patch, I'm not convinced that this is necessary, since I suspect that the code that calls toSelect() might set the forUpdate boolean based on the values in the JDBCFetchConfiguration anyways.) 2. an isolation-override setting. Currently, this patch implements all of this only in the DB2Dictionary. As of right now, the first feature is something that is useful for all sorts of databases, via syntax like Oracle's SELECT ... FOR UPDATE. However, we only know how to implement the second feature for DB2, and not for any other database. In Oracle, "ALTER SESSION" can be used to change the isolation level of a given session, but I'm not sure of the semantics of this operation. I believe that Abe's question is: Do other databases (Sybase? Derby?) also have semantics for changing the isolation level of a particular query? I have no practical experience with Sybase, but I did find this in their TransactSQL user's guide :
"Changing the isolation level for a query You can change the isolation level for a query by using the at isolation clause with the select or readtext statements. The at isolation clause supports isolation levels 0, 1, and 3. It does not support isolation level 2. The read uncommitted, read committed, and serializable options of at isolation represent isolation levels as listed below: at isolation option Isolation level read uncommited 0 read committed 1 serializable 3 For example, the following two statements query the same table at isolation levels 0 and 3, respectively: select * from titles at isolation read uncommitted select * from titles at isolation serializable" There's more information online here: http://manuals.sybase.com/onlinebooks/group-as/asg1250e/sqlug/@Generic__BookTextView/53911;hf=0 Between Ritika's SQLServer email on the dev list and Mike's Sybase research, it sounds like this feature is supported by enough databases that I think it's useful to expose as an API, rather than just a DB2-specific hint.
Here is a jar containing the 2 entities on whcih I run my testcases.
em.getTransaction().begin() Query qryA = em.createQuery("select d from DeptBean2 d where d.no = 1"); qryA.setHint("openjpa.hint.updateClause",true); qryA.setHint("openjpa.hint.isolationLevel", "serializable"); List rsA = qryA.getResultList(); The SQL Output looks like 13109 dwtest TRACE [main] openjpa.jdbc.SQL - <t 1094730048, conn 1593466618> executing prepstmnt 694036830 SELECT t0.no, t1.empid, t2.no, t2.mgr_empid, t2.cdname, t1.ceemp_ts, t1.cehireDate, t1.ceismanager, t1.cename, t1.cesalary, t0.cdname FROM deptab2 t0 LEFT OUTER JOIN emptab2 t1 ON t0.mgr_empid = t1.empid LEFT OUTER JOIN deptab2 t2 ON t1.dept_no = t2.no WHERE (CAST(t0.no AS BIGINT) = CAST(? AS BIGINT)) WITH RR USE AND KEEP UPDATE LOCKS [params=(long) 1] 13119 dwtest TRACE [main] openjpa.jdbc.SQL - <t 1094730048, conn 1593466618> [10 ms] spent 27420 dwtest TRACE [main] openjpa.jdbc.SQL - <t 1094730048, conn 1593466618> executing prepstmnt 2102295886 SELECT t0.empid, t1.no, t2.empid, t2.dept_no, t2.ceemp_ts, t2.cehireDate, t2.ceismanager, t2.cename, t2.cesalary, t1.cdname, t0.ceemp_ts, t0.cehireDate, t0.ceismanager, t0.cename, t0.cesalary FROM emptab2 t0 LEFT OUTER JOIN deptab2 t1 ON t0.dept_no = t1.no LEFT OUTER JOIN emptab2 t2 ON t1.mgr_empid = t2.empid WHERE t0.dept_no = ? WITH RR USE AND KEEP UPDATE LOCKS [params=(int) 1] 27430 dwtest TRACE [main] openjpa.jdbc.SQL - <t 1094730048, conn 1593466618> [10 ms] spent Query qryB = em.createQuery("select e from EmpBean2 e where e.empid = 1"); qryB.setHint("openjpa.hint.updateClause",true); qryB.setHint("openjpa.hint.isolationLevel", "read-uncommitted"); List rsB = qryB.getResultList(); The SQL Output looks like 47969 dwtest TRACE [main] openjpa.jdbc.SQL - <t 1094730048, conn 1593466618> executing prepstmnt 1530944320 SELECT t0.empid, t1.no, t2.empid, t2.dept_no, t2.ceemp_ts, t2.cehireDate, t2.ceismanager, t2.cename, t2.cesalary, t1.cdname, t0.ceemp_ts, t0.cehireDate, t0.ceismanager, t0.cename, t0.cesalary FROM emptab2 t0 LEFT OUTER JOIN deptab2 t1 ON t0.dept_no = t1.no LEFT OUTER JOIN emptab2 t2 ON t1.mgr_empid = t2.empid WHERE (CAST(t0.empid AS BIGINT) = CAST(? AS BIGINT)) WITH RS USE AND KEEP UPDATE LOCKS [params=(long) 1] 47969 dwtest TRACE [main] openjpa.jdbc.SQL - <t 1094730048, conn 1593466618> [0 ms] spent Query qryC = em.createQuery("select d from DeptBean2 d where d.no = 1"); DeptBean2 deptC = (DeptBean2)qryC.getSingleResult(); The SQL Out put is 72695 dwtest TRACE [main] openjpa.jdbc.SQL - <t 1094730048, conn 1593466618> executing prepstmnt 742009914 SELECT t0.no, t1.empid, t2.no, t2.mgr_empid, t2.cdname, t1.ceemp_ts, t1.cehireDate, t1.ceismanager, t1.cename, t1.cesalary, t0.cdname FROM deptab2 t0 LEFT OUTER JOIN emptab2 t1 ON t0.mgr_empid = t1.empid LEFT OUTER JOIN deptab2 t2 ON t1.dept_no = t2.no WHERE (CAST(t0.no AS BIGINT) = CAST(? AS BIGINT)) FOR READ ONLY optimize for 1 row [params=(long) 1] 72695 dwtest TRACE [main] openjpa.jdbc.SQL - <t 1094730048, conn 1593466618> [0 ms] spent In my persistence.xml I had the following properties <property name="openjpa.LockManager" value="pessimistic" /> <property name="openjpa.jdbc.TransactionIsolation" value="read-committed" /> Essentially if we are running against DB2 8.2 or Later then if update lock is true then for all the isolation levels other than "serializable" WITH RS USE AND KEEP UPDATE LOCKS clause is appended to the query.In case of "serializable" isolation level "WITH RR USE AND KEEP UPDATE LOCK" is appended. If the updateLock is false then FOR READ ONLY clause is appended to all queries.
Ritika Maheshwari made changes - 05/Apr/07 09:06 PM
Michael Dick made changes - 06/Apr/07 12:50 PM
Remove forUpdate from DBDictionary.toOperation()'s signature; use a null check on forUpdateClause instead.
Patrick Linskey made changes - 06/Apr/07 06:45 PM
I think that, aside from documentation and DB2 test cases and impls for other data stores, we can put this issue to rest.
Patrick Linskey made changes - 06/Apr/07 11:54 PM
we just discovered that there is a problem with this fix.The problem is that the FOR READ ONLY flag is getting generated for subselects as well.
Basically for DB2 if forUpdate was false we append FOR READ ONLY we need another check to see that it is not a subselect.To me it appears that in the toSelect method when SelectImpl is passed we check that the parent is null or not to figure out that it is a subselect and then pass another flag subselect to the getForUdateClause method where we say if(!forUpdate && !subselect) forUpdateString.append(forReadOnlyClause) to achieve this we would probably have to override the toSelect methods in DB2Dictionary again unless there is a better way. I am attaching a patch to specify the changes
Ritika Maheshwari made changes - 17/Apr/07 09:43 PM
Some comments:
1. I don't think that we should be doing work on resolved issues. So, this should be re-opened, or (preferably) a new issue should be opened for this new bug. 2. The patch you attached does not use OpenJPA-style formatting. We don't have a style guide spelled out as well as we probably should, but we always put spaces after commas, we indent 4 spaces on continuation lines, and we put a space between an 'if' and the open paren. 3. It's a shame to have to do all this code duplication between DBDictionary and DB2Dictionary. To what extent can we refactor DBDictionary's methods to make this concept work out better for DB2Dictionary?
Michael Dick made changes - 18/Apr/07 08:22 PM
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||