OpenJPA
  1. OpenJPA
  2. OPENJPA-1604

Setting PessimisticLockManager fails to append "for update clause" to the select statement

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0
    • Fix Version/s: 2.0.0
    • Component/s: None
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      I ran a testcase against openjpa 1.2, and found that the "for update" clause is appended to the SQL when

      <property name="openjpa.LockManager" value="pessimistic"/>

      is added to the persistence.xml without calling:

      q.setLockMode(LockModeType.PESSIMISTIC_WRITE);

      However, this behavior changes when running against trunk level code

      1. OPENJPA-1604_optimistic_fix_2.0.x.patch
        8 kB
        Jeremy Bauer
      2. OPENJPA-1604-2.0.x.patch
        14 kB
        Rick Curtis
      3. OPENJPA-1604-trunk.patch
        14 kB
        Rick Curtis

        Issue Links

          Activity

          Fay Wang created issue -
          Hide
          Pinaki Poddar added a comment -

          The setting of lock modes based on current lock manager seems to be ambiguous at few places both in usage and in design/code.

          From a usage point of view,
          a) setting a specific lock manager be it pessimistic or optimistic should impose a specific default read and write lock level for all queries.
          b) that default level can be overwritten by
          i) setting query.setLockMode()
          ii) setting lockMode attribute of @NamedQuery
          iii) setting query hints
          iv) passing lock properties in find() etc.

          c) the semantics of a joint specification such as
          <property name="openjpa.LockManager" value="pessimistic"/>
          <property name="openjpa.Optimistc" value="true"/>
          also appears confusing/contradictory. We should either detect specification that are contradictory or clarify what such apparently contradictory specifications imply.

          From a design/code point of view,
          a) setting a FetchPlan.Lock hint during annotation parsing does not look good to me. We have reasonable data structures to capture meta information on a query and that seems to be the right place for lock information rather than a fetch plan hint.
          b) the default read/write lock levels should be dictated by the currently active lock manager. LockManager.beginTransaction() seems to be one correct place to populate the fetch configuration with these levels. Outside transaction, fetch plan initialization should take current Lock Manager into consideration to set its read/write lock levels.
          c) OpenJPAConfiguration should impose consistency between LockManager and setOptimistic()

          Show
          Pinaki Poddar added a comment - The setting of lock modes based on current lock manager seems to be ambiguous at few places both in usage and in design/code. From a usage point of view, a) setting a specific lock manager be it pessimistic or optimistic should impose a specific default read and write lock level for all queries. b) that default level can be overwritten by i) setting query.setLockMode() ii) setting lockMode attribute of @NamedQuery iii) setting query hints iv) passing lock properties in find() etc. c) the semantics of a joint specification such as <property name="openjpa.LockManager" value="pessimistic"/> <property name="openjpa.Optimistc" value="true"/> also appears confusing/contradictory. We should either detect specification that are contradictory or clarify what such apparently contradictory specifications imply. From a design/code point of view, a) setting a FetchPlan.Lock hint during annotation parsing does not look good to me. We have reasonable data structures to capture meta information on a query and that seems to be the right place for lock information rather than a fetch plan hint. b) the default read/write lock levels should be dictated by the currently active lock manager. LockManager.beginTransaction() seems to be one correct place to populate the fetch configuration with these levels. Outside transaction, fetch plan initialization should take current Lock Manager into consideration to set its read/write lock levels. c) OpenJPAConfiguration should impose consistency between LockManager and setOptimistic()
          Donald Woods made changes -
          Field Original Value New Value
          Assignee Pinaki Poddar [ ppoddar@apache.org ]
          Fix Version/s 2.0.0 [ 12314019 ]
          Fix Version/s 2.1.0 [ 12314542 ]
          Affects Version/s 2.0.0 [ 12314019 ]
          Patch Info [Patch Available]
          Hide
          Rick Curtis added a comment -

          After digging into this problem I concur with Pinaki's assessment that the lock modes / lock configuration has numerous issues. I'm posting a patch that will only address the backward compatibility issue.

          The root issue at hand is that in JPA 1.0 a named query had a default lock mode of READ (by OpenJPA's definition, not the spec), but per the 2.0 spec the default lock mode is NONE.

          Ideally when we are parsing annotations in AnnotationPersistenceMetaDataParser we could differentiate between a named query which has specified the lockMode vs a default lockMode. Since that doesn't seem to be possible, I'm proposing that if we detect that we're using a pessimistic lock manager and the lockMode is none, we will promote that lock to a READ lock.

          This approach will restore backward compatibility, but will exclude one use case. When using a pessimistic lock manager on 2.0 you will be unable to configure a NamedQuery with the lockMode of NONE.

          Show
          Rick Curtis added a comment - After digging into this problem I concur with Pinaki's assessment that the lock modes / lock configuration has numerous issues. I'm posting a patch that will only address the backward compatibility issue. The root issue at hand is that in JPA 1.0 a named query had a default lock mode of READ (by OpenJPA's definition, not the spec), but per the 2.0 spec the default lock mode is NONE. Ideally when we are parsing annotations in AnnotationPersistenceMetaDataParser we could differentiate between a named query which has specified the lockMode vs a default lockMode. Since that doesn't seem to be possible, I'm proposing that if we detect that we're using a pessimistic lock manager and the lockMode is none, we will promote that lock to a READ lock. This approach will restore backward compatibility, but will exclude one use case. When using a pessimistic lock manager on 2.0 you will be unable to configure a NamedQuery with the lockMode of NONE.
          Rick Curtis made changes -
          Attachment OPENJPA-1604-2.0.x.patch [ 12441191 ]
          Attachment OPENJPA-1604-trunk.patch [ 12441192 ]
          Hide
          Rick Curtis added a comment -

          I just noticed that my patch still has some test debug in it... I'll update it shortly.

          Show
          Rick Curtis added a comment - I just noticed that my patch still has some test debug in it... I'll update it shortly.
          Rick Curtis made changes -
          Attachment OPENJPA-1604-2.0.x.patch [ 12441191 ]
          Rick Curtis made changes -
          Attachment OPENJPA-1604-trunk.patch [ 12441192 ]
          Rick Curtis made changes -
          Attachment OPENJPA-1604-trunk.patch [ 12441198 ]
          Attachment OPENJPA-1604-2.0.x.patch [ 12441199 ]
          Rick Curtis made changes -
          Assignee Pinaki Poddar [ ppoddar@apache.org ] Rick Curtis [ curtisr7 ]
          Hide
          Pinaki Poddar added a comment -

          Some of the tests such as executing a NamedQuery with lock outside a transaction should raise an error has been @AllowFailured.
          Please add the reason/explanatory texts to @AllowFailure why that is done/required.

          Show
          Pinaki Poddar added a comment - Some of the tests such as executing a NamedQuery with lock outside a transaction should raise an error has been @AllowFailured. Please add the reason/explanatory texts to @AllowFailure why that is done/required.
          Donald Woods made changes -
          Fix Version/s 2.1.0 [ 12314542 ]
          Affects Version/s 2.1.0 [ 12314542 ]
          Hide
          Donald Woods added a comment -

          Code checked into 2.0.0, so marking it resolved for the release notes.
          If more work is required, please open a new issue and link it to this one.

          Show
          Donald Woods added a comment - Code checked into 2.0.0, so marking it resolved for the release notes. If more work is required, please open a new issue and link it to this one.
          Donald Woods made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Rick Curtis made changes -
          Link This issue is related to OPENJPA-1629 [ OPENJPA-1629 ]
          Hide
          Rick Curtis added a comment -

          @Pinaki – See OPENJPA-1629 regarding the @AllowFailure.

          Show
          Rick Curtis added a comment - @Pinaki – See OPENJPA-1629 regarding the @AllowFailure.
          Donald Woods made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Hide
          Jeremy Bauer added a comment -

          Attaching a patch for 2.0.x which takes into account the Optimistic setting when calculating the default lock mode to apply to a named query. This must occur in order to maintain 1.x behavior.

          Show
          Jeremy Bauer added a comment - Attaching a patch for 2.0.x which takes into account the Optimistic setting when calculating the default lock mode to apply to a named query. This must occur in order to maintain 1.x behavior.
          Jeremy Bauer made changes -
          Attachment OPENJPA-1604_optimistic_fix_2.0.x.patch [ 12442891 ]
          Michael Dick made changes -
          Link This issue incorporates OPENJPA-1714 [ OPENJPA-1714 ]

            People

            • Assignee:
              Rick Curtis
              Reporter:
              Fay Wang
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development