OpenJPA
  1. OpenJPA
  2. OPENJPA-235

SQL reordering to avoid non-nullable foreign key constraint violations

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.1.1, 1.2.0, 1.3.0
    • Component/s: kernel
    • Labels:
      None

      Description

      OpenJPA does not do any SQL statement re-ordering in order to resolve foreign key constraints. Instead, objects are always inserted in the order in which the user persists the instances. When you persist in an order that would violate foreign key constraints, OpenJPA attempts to insert null and then update the foreign key value in a separate statement. If you use non-nullable constraints, though, you must persist your objects in the correct order.

      This improvement re-orders SQL statements as follows:

      1. First, all insert statements execute. Inserts which have foreign keys with non-nullable constraints execute AFTER the foreign keys which they depend on have been inserted since no deferred update is possible.

      2. Next, all update statements execute. No reordering is necessary.

      3. Finally, all delete statements execute. Like inserts, deletes execute in an order which does not violate non-nullable foreign key constraints.

      If a circular foreign key reference is found during the re-ordering process then re-ordering halts and the remaining unordered statements are left as is. There is nothing that can be done about the circular reference (other than fixing the schema) and the resulting SQL statements will not succeed.

      The net effect is that users do not need to worry about the persistence order of their objects regardless of non-nullable foreign key constraints. The only class modified was org.apache.openjpa.jdbc.kernel.OperationOrderUpdateManager. I have included a patch which includes my modifications to OperationOrderUpdateManager and test cases. The test cases I have provided fail on the current trunk but pass with my modifications. I have also verified that I did not break anything by using maven to run all test cases with my modifications in place.

      1. merge-detached.patch
        0.8 kB
        Gokhan Ergul
      2. merge-testcases.patch
        18 kB
        Gokhan Ergul
      3. openjpa.patch
        2 kB
        Fay Wang
      4. openjpa.patch
        2 kB
        Fay Wang
      5. openjpa-235-break-nullable.patch
        40 kB
        Markus Fuchs
      6. openjpa-235-break-nullable-070716.patch
        43 kB
        Markus Fuchs
      7. openjpa-235-delete-cascade.patch
        9 kB
        Fay Wang
      8. openjpa-235-test.jar
        8 kB
        Markus Fuchs
      9. openjpa-235-test1.jar
        8 kB
        Markus Fuchs
      10. openjpa-235-test2.zip
        6 kB
        Markus Fuchs
      11. sqlreorder.patch
        21 kB
        Reece Garrett
      12. sqlReorder2.patch
        32 kB
        Reece Garrett
      13. sqlReorderTests.patch
        11 kB
        Reece Garrett

        Issue Links

          Activity

          Reece Garrett created issue -
          Hide
          Reece Garrett added a comment -

          Patch includes code changes and test cases.

          Show
          Reece Garrett added a comment - Patch includes code changes and test cases.
          Reece Garrett made changes -
          Field Original Value New Value
          Attachment sqlreorder.patch [ 12356797 ]
          Reece Garrett made changes -
          Link This issue is related to OPENJPA-177 [ OPENJPA-177 ]
          Hide
          Reece Garrett added a comment -

          This bug illustrates the effects of not re-ordering sql when dealing with non-nullable foreign key constraints. The patch provided with this improvement fixes these issues.

          Show
          Reece Garrett added a comment - This bug illustrates the effects of not re-ordering sql when dealing with non-nullable foreign key constraints. The patch provided with this improvement fixes these issues.
          Patrick Linskey made changes -
          Assignee Patrick Linskey [ pcl ]
          Patrick Linskey made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Marc Prud'hommeaux added a comment -

          I need to revert the patch for OperationOrderUpdateManager, since it is causing 19 TCK failures (one of the test failures can be reproduced with: mvn integration-test -Dtest=false -Ptck-profile -Djpatck.pkg.dir=com/sun/ts/tests/ejb30/persistence/annotations/orderby ).

          Show
          Marc Prud'hommeaux added a comment - I need to revert the patch for OperationOrderUpdateManager, since it is causing 19 TCK failures (one of the test failures can be reproduced with: mvn integration-test -Dtest=false -Ptck-profile -Djpatck.pkg.dir=com/sun/ts/tests/ejb30/persistence/annotations/orderby ).
          Marc Prud'hommeaux made changes -
          Status Resolved [ 5 ] Reopened [ 4 ]
          Resolution Fixed [ 1 ]
          Hide
          Patrick Linskey added a comment -

          Any idea about the nature of the failure? Was the patch reordering things incorrectly?

          Ideally, we should fix this in a way that passes the TCK; did it seem like a significant enough problem that it's worthwhile to create a separate non-default UpdateManager for use by the TCK (or for use to access the feature)?

          Show
          Patrick Linskey added a comment - Any idea about the nature of the failure? Was the patch reordering things incorrectly? Ideally, we should fix this in a way that passes the TCK; did it seem like a significant enough problem that it's worthwhile to create a separate non-default UpdateManager for use by the TCK (or for use to access the feature)?
          Hide
          Marc Prud'hommeaux added a comment -

          I didn't look to closely at the failures. I just saw that the TCK hadn't been passing since May 4th, so I figured it would be best to restore its passing status, and the patch author could work to determine why the failures were occurring. I do recall that the errors I saw had to do with foreign key constraint violations, which was a little surprising (since the patch was designed to avoid precisely that).

          The errors are pretty easily reproducable with the command I mention above, provided you have access to the TCK.

          Show
          Marc Prud'hommeaux added a comment - I didn't look to closely at the failures. I just saw that the TCK hadn't been passing since May 4th, so I figured it would be best to restore its passing status, and the patch author could work to determine why the failures were occurring. I do recall that the errors I saw had to do with foreign key constraint violations, which was a little surprising (since the patch was designed to avoid precisely that). The errors are pretty easily reproducable with the command I mention above, provided you have access to the TCK.
          Hide
          Gokhan Ergul added a comment -

          FWIW, I've tested the patch with the hope that it'd also fix OPENJPA-231, however it broke a good number of my local tests which persist fresh objects (with multiple generations of cascading collections), I couldn't get to the stage where I can test merge behaviour at all --had to rollback locally.

          Show
          Gokhan Ergul added a comment - FWIW, I've tested the patch with the hope that it'd also fix OPENJPA-231 , however it broke a good number of my local tests which persist fresh objects (with multiple generations of cascading collections), I couldn't get to the stage where I can test merge behaviour at all --had to rollback locally.
          Hide
          Reece Garrett added a comment -

          I have been doing further work on this patch over the past week to deal with several edge cases I did not initially think of (which may have caused the failures) and to deal with unique constraint violations. I will reproduce and fix the tests Marc referred to.

          Gokhan, I am interested in the test cases that this broke so if you have time please email me.

          I apologize for any inconvenience this has caused but I was not aware of additional test cases that I needed to run. This functionality is sorely needed for anyone using a database which does not support deferred constraints so I will continue to work on it.

          Also, I just posted a message on the mailing list, "SQL ordering and unique constraints", and would appreciate any suggestions on how to proceed.

          Show
          Reece Garrett added a comment - I have been doing further work on this patch over the past week to deal with several edge cases I did not initially think of (which may have caused the failures) and to deal with unique constraint violations. I will reproduce and fix the tests Marc referred to. Gokhan, I am interested in the test cases that this broke so if you have time please email me. I apologize for any inconvenience this has caused but I was not aware of additional test cases that I needed to run. This functionality is sorely needed for anyone using a database which does not support deferred constraints so I will continue to work on it. Also, I just posted a message on the mailing list, "SQL ordering and unique constraints", and would appreciate any suggestions on how to proceed.
          Hide
          Reece Garrett added a comment -

          I am just about done revising my patch but I do not have access to the TCK and can not sign the NDA because I work for a government agency which will not allow it. All the test cases I can run (mvn test) pass but that's not saying much because they passed the first time. As I said, I have caught a few cases that I did not think of the first time and am hopeful that they were the source of the TCK failures but can not be sure. Also, the algorithm is now iterative instead of recursive, does almost no collection copying, and orders rows based purely on dependencies instead of lumping all inserts first followed by updates and then deletes. I decided not to include support for ordering unique keys until I get foreign key ordering resolved (although I'm fairly certain my idea for unique keys will work well.

          I'm at a loss as to how to proceed from here. I'll have a new patch ready soon but since I can not run the TCK tests myself I don't feel comfortable resubmitting it and potentially taking up more of your development time tracking down bugs I've introduced. What should I do with my new patch?

          Show
          Reece Garrett added a comment - I am just about done revising my patch but I do not have access to the TCK and can not sign the NDA because I work for a government agency which will not allow it. All the test cases I can run (mvn test) pass but that's not saying much because they passed the first time. As I said, I have caught a few cases that I did not think of the first time and am hopeful that they were the source of the TCK failures but can not be sure. Also, the algorithm is now iterative instead of recursive, does almost no collection copying, and orders rows based purely on dependencies instead of lumping all inserts first followed by updates and then deletes. I decided not to include support for ordering unique keys until I get foreign key ordering resolved (although I'm fairly certain my idea for unique keys will work well. I'm at a loss as to how to proceed from here. I'll have a new patch ready soon but since I can not run the TCK tests myself I don't feel comfortable resubmitting it and potentially taking up more of your development time tracking down bugs I've introduced. What should I do with my new patch?
          Hide
          Markus Fuchs added a comment -

          Hi Reece,

          This test case produces a foreign key violation when applying your previous patch.

          – markus.

          Show
          Markus Fuchs added a comment - Hi Reece, This test case produces a foreign key violation when applying your previous patch. – markus.
          Markus Fuchs made changes -
          Attachment openjpa-235-test.jar [ 12358172 ]
          Hide
          Patrick Linskey added a comment -

          Hi,

          Attach the patch to the JIRA and one of those of us with TCK access will run it against the TCK.

          Show
          Patrick Linskey added a comment - Hi, Attach the patch to the JIRA and one of those of us with TCK access will run it against the TCK.
          Hide
          Reece Garrett added a comment -

          Markus,

          Thank you for your test case. It runs just fine with my new code. I will post a new patch on Tuesday. Everyone have a great weekend!

          Show
          Reece Garrett added a comment - Markus, Thank you for your test case. It runs just fine with my new code. I will post a new patch on Tuesday. Everyone have a great weekend!
          Hide
          Markus Fuchs added a comment -

          Hi Reece,

          Thanks for trying my test case. I've been playing around with commit dependencies, and this is, what I found:

          1) One reason, why your previous patch didn't work is, as you already found out, that the rows returned by RowManagerImpl.getInserts returns are not in user order. This might be unexpected and should either be documented or changed (I haven't found any usages of RowManagerImpl.getInserts in the current code, so it might be possible to change that). An alternative would be to add a new method RowManagerImpl.getOrderedInserts.

          2) Another situation, that generally doesn't work (both w/ your patch and w/o), is when relationships are nullified before the objects are deleted. Please see my new test openjpa-235-test1. OpenJPA doesn't execute updates for deleted objects. There will be a fk constraint violation if the updates are skipped and the objects aren't modified according to the constraint dependencies inside the user transaction. I couldn't find how to get "old" relationship value before the update, which would be needed for the dependency calculation.

          3) It seems, that join table entries are removed from the non-owning side. IMHO, relationship updates should only be done from the owning side.

          Show
          Markus Fuchs added a comment - Hi Reece, Thanks for trying my test case. I've been playing around with commit dependencies, and this is, what I found: 1) One reason, why your previous patch didn't work is, as you already found out, that the rows returned by RowManagerImpl.getInserts returns are not in user order. This might be unexpected and should either be documented or changed (I haven't found any usages of RowManagerImpl.getInserts in the current code, so it might be possible to change that). An alternative would be to add a new method RowManagerImpl.getOrderedInserts. 2) Another situation, that generally doesn't work (both w/ your patch and w/o), is when relationships are nullified before the objects are deleted. Please see my new test openjpa-235-test1. OpenJPA doesn't execute updates for deleted objects. There will be a fk constraint violation if the updates are skipped and the objects aren't modified according to the constraint dependencies inside the user transaction. I couldn't find how to get "old" relationship value before the update, which would be needed for the dependency calculation. 3) It seems, that join table entries are removed from the non-owning side. IMHO, relationship updates should only be done from the owning side.
          Markus Fuchs made changes -
          Attachment openjpa-235-test1.jar [ 12358293 ]
          Hide
          Reece Garrett added a comment -

          I've posted my updated patch (sqlReorder2.patch). It is considerably different from the first patch. Statements are ordered based on dependency to other statements. The logic is as follows:

          INSERTS: an insert statement depends on other insert statements involving rows with a non-nullable foreign key to it. If the foreign key is nullable and it is necessary to avoid a constraint violation then the foreign key columns are initially inserted as null and later updated after the foreign key has been inserted.

          UPDATES: an update statement depends on insert statements involving rows with a non-nullable foreign key to it. If the foreign key is nullable and it is necessary to avoid a constraint violation then the foreign key columns are initially updated to null and later updated again after the foreign key has been inserted.

          DELETES: A delete statement depends on other delete statements involving rows with a non-nullable foreign key to it. If the foreign key is nullable and it is necessary to avoid a constraint violation then an update is generated to null the offending foreign key before this delete executes. A delete statement may be generated because another delete or update dereferenced it's state. If this is the case then said delete depends on the update or delete that caused the dereferenced state.

          To calculate dependencies for dereferenced states it is necessary to store those states in the state that dereferenced them. This involved adding a collection (set) to each state, methods to add and remove states to the collection, and a method to retrieve the states from the collection. Of the three methods mentioned only the method that retrieves the dereferenced states was added to the OpenJPAStateManager interface. I had to add the method to the interface because OperationOrderUpdateManager (where I calculate dependencies) programs to that interface, however, SingleFieldManager, where the calls to dereference and un-dereference states initiate) does not. SingleFieldManager programs to StateManagerImpl which is an implementation of OpenJPAStatManager. Of course that did not prevent me from adding the add and remove methods to the OpenJPAStateManager interface but there was no reason to.

          Adding the one method to the OpenJPAStateManager interface forced me to change all implementing classes including DetatchedStateManager, DetachedValueStateManager, ObjectIdStateManager, NullEmbeddedStateManager (an inner class of EmbededFieldStrategy), and of course StateManagerImpl. StateManagerImpl is the only implementation that actually uses the method so the rest of classes throw either an UnsupportedOperationException or an InternalException.

          I should also mention that I consider updating a persistence capable field to null to be dereferencing it and added the state for that persistence capable object to the collection of dereferenced states.

          I have provided a simple test case to exercise my code and have verified that all other OpenJPA test cases pass (with the exception of the TCK tests which I do not have access to). Without being able to run the TCK tests I cannot guarantee that I have addressed the issues that caused the previous failures, but I have caught several cases that I did not catch the first time so I am confident.

          Markus,

          Again, thank you for the test cases. They all pass and I have addressed all of your issues you mentioned in your last post except for your comment about deletes from join tables. Join tables are secondary tables and are handled up front by OpenJPA so I don't mess with any of that.

          Show
          Reece Garrett added a comment - I've posted my updated patch (sqlReorder2.patch). It is considerably different from the first patch. Statements are ordered based on dependency to other statements. The logic is as follows: INSERTS: an insert statement depends on other insert statements involving rows with a non-nullable foreign key to it. If the foreign key is nullable and it is necessary to avoid a constraint violation then the foreign key columns are initially inserted as null and later updated after the foreign key has been inserted. UPDATES: an update statement depends on insert statements involving rows with a non-nullable foreign key to it. If the foreign key is nullable and it is necessary to avoid a constraint violation then the foreign key columns are initially updated to null and later updated again after the foreign key has been inserted. DELETES: A delete statement depends on other delete statements involving rows with a non-nullable foreign key to it. If the foreign key is nullable and it is necessary to avoid a constraint violation then an update is generated to null the offending foreign key before this delete executes. A delete statement may be generated because another delete or update dereferenced it's state. If this is the case then said delete depends on the update or delete that caused the dereferenced state. To calculate dependencies for dereferenced states it is necessary to store those states in the state that dereferenced them. This involved adding a collection (set) to each state, methods to add and remove states to the collection, and a method to retrieve the states from the collection. Of the three methods mentioned only the method that retrieves the dereferenced states was added to the OpenJPAStateManager interface. I had to add the method to the interface because OperationOrderUpdateManager (where I calculate dependencies) programs to that interface, however, SingleFieldManager, where the calls to dereference and un-dereference states initiate) does not. SingleFieldManager programs to StateManagerImpl which is an implementation of OpenJPAStatManager. Of course that did not prevent me from adding the add and remove methods to the OpenJPAStateManager interface but there was no reason to. Adding the one method to the OpenJPAStateManager interface forced me to change all implementing classes including DetatchedStateManager, DetachedValueStateManager, ObjectIdStateManager, NullEmbeddedStateManager (an inner class of EmbededFieldStrategy), and of course StateManagerImpl. StateManagerImpl is the only implementation that actually uses the method so the rest of classes throw either an UnsupportedOperationException or an InternalException. I should also mention that I consider updating a persistence capable field to null to be dereferencing it and added the state for that persistence capable object to the collection of dereferenced states. I have provided a simple test case to exercise my code and have verified that all other OpenJPA test cases pass (with the exception of the TCK tests which I do not have access to). Without being able to run the TCK tests I cannot guarantee that I have addressed the issues that caused the previous failures, but I have caught several cases that I did not catch the first time so I am confident. Markus, Again, thank you for the test cases. They all pass and I have addressed all of your issues you mentioned in your last post except for your comment about deletes from join tables. Join tables are secondary tables and are handled up front by OpenJPA so I don't mess with any of that.
          Reece Garrett made changes -
          Attachment sqlReorder2.patch [ 12358573 ]
          Hide
          Gokhan Ergul added a comment -

          Reece,

          I'm sorry it took so long, somehow I managed to mess up maven plugin repo, wasn't able to run tests at all for the last week. Testcases attached, they're ready to be used as part of standard openjpa tests. TestMultigenOneToManyMerge.updateDetached should succeed with the patch I attached. TestMultigenOneToManyMerge.updateAttached works with auto-generated tables against Derby, fails against mysql/innodb with foreign keys defined between tables, that's the case I'm hoping your patch would solve.

          Let me know if you have trouble setting up/running the tests.

          Gokhan.

          Show
          Gokhan Ergul added a comment - Reece, I'm sorry it took so long, somehow I managed to mess up maven plugin repo, wasn't able to run tests at all for the last week. Testcases attached, they're ready to be used as part of standard openjpa tests. TestMultigenOneToManyMerge.updateDetached should succeed with the patch I attached. TestMultigenOneToManyMerge.updateAttached works with auto-generated tables against Derby, fails against mysql/innodb with foreign keys defined between tables, that's the case I'm hoping your patch would solve. Let me know if you have trouble setting up/running the tests. Gokhan.
          Gokhan Ergul made changes -
          Attachment merge-multigen-collection-testcase.zip [ 12358588 ]
          Attachment merge-detached.patch [ 12358589 ]
          Hide
          Reece Garrett added a comment -

          Gokhan,

          Thank you for the test cases. The first test case ( testMergeAttached ) passes but the other case ( testMergeDetached ) fails before it even gets to my code. I see what you're trying to do and my patch should handle it. I'm sorry I don't have time to tell you why the error occurs but I suspect it is a problem with your annotations. Here is the error testMergeDetached generates:

          <0.0.0 nonfatal user error> org.apache.openjpa.persistence.ArgumentException: Encountered new object "org.apache.openjpa.persistence.merge.model.EntityB@1450f1f" in persistent field "org.apache.openjpa.persistence.merge.model.EntityC.parent" of managed object "org.apache.openjpa.persistence.merge.model.EntityC@1a722ef" during attach. However, this field does not allow cascade attach. You cannot attach a reference to a new object without cascading.
          FailedObject: org.apache.openjpa.persistence.merge.model.EntityB@1450f1f
          at org.apache.openjpa.kernel.AttachStrategy.getReference(AttachStrategy.java:276)
          at org.apache.openjpa.kernel.AttachStrategy.attachField(AttachStrategy.java:193)
          at org.apache.openjpa.kernel.VersionAttachStrategy.attach(VersionAttachStrategy.java:134)
          at org.apache.openjpa.kernel.AttachManager.attach(AttachManager.java:239)
          at org.apache.openjpa.kernel.AttachStrategy.attachCollection(AttachStrategy.java:330)
          at org.apache.openjpa.kernel.AttachStrategy.replaceCollection(AttachStrategy.java:298)
          at org.apache.openjpa.kernel.AttachStrategy.attachField(AttachStrategy.java:220)
          at org.apache.openjpa.kernel.VersionAttachStrategy.attach(VersionAttachStrategy.java:134)
          at org.apache.openjpa.kernel.AttachManager.attach(AttachManager.java:239)
          at org.apache.openjpa.kernel.AttachStrategy.attachCollection(AttachStrategy.java:330)
          at org.apache.openjpa.kernel.DetachedStateManager.attach(DetachedStateManager.java:250)
          at org.apache.openjpa.kernel.AttachManager.attach(AttachManager.java:239)
          at org.apache.openjpa.kernel.AttachManager.attach(AttachManager.java:100)
          at org.apache.openjpa.kernel.BrokerImpl.attach(BrokerImpl.java:3176)
          at org.apache.openjpa.kernel.DelegatingBroker.attach(DelegatingBroker.java:1147)
          at org.apache.openjpa.persistence.EntityManagerImpl.merge(EntityManagerImpl.java:665)
          at org.piercecountywa.personnel.openjpa.TestMultigenOneToManyMerge.testMergeDetached(TestMultigenOneToManyMerge.java:108)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
          at java.lang.reflect.Method.invoke(Unknown Source)
          at junit.framework.TestCase.runTest(TestCase.java:164)
          at junit.framework.TestCase.runBare(TestCase.java:130)
          at junit.framework.TestResult$1.protect(TestResult.java:110)
          at junit.framework.TestResult.runProtected(TestResult.java:128)
          at junit.framework.TestResult.run(TestResult.java:113)
          at junit.framework.TestCase.run(TestCase.java:120)
          at junit.framework.TestSuite.runTest(TestSuite.java:228)
          at junit.framework.TestSuite.run(TestSuite.java:223)
          at org.junit.internal.runners.OldTestClassRunner.run(OldTestClassRunner.java:35)
          at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:38)
          at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
          at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:460)
          at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:673)
          at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:386)
          at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:196)

          Again, the good news is the first test case runs great and what you're trying to do in the second test should work great too. If you can figure out what the issue is on that second test please post it.

          Show
          Reece Garrett added a comment - Gokhan, Thank you for the test cases. The first test case ( testMergeAttached ) passes but the other case ( testMergeDetached ) fails before it even gets to my code. I see what you're trying to do and my patch should handle it. I'm sorry I don't have time to tell you why the error occurs but I suspect it is a problem with your annotations. Here is the error testMergeDetached generates: <0.0.0 nonfatal user error> org.apache.openjpa.persistence.ArgumentException: Encountered new object "org.apache.openjpa.persistence.merge.model.EntityB@1450f1f" in persistent field "org.apache.openjpa.persistence.merge.model.EntityC.parent" of managed object "org.apache.openjpa.persistence.merge.model.EntityC@1a722ef" during attach. However, this field does not allow cascade attach. You cannot attach a reference to a new object without cascading. FailedObject: org.apache.openjpa.persistence.merge.model.EntityB@1450f1f at org.apache.openjpa.kernel.AttachStrategy.getReference(AttachStrategy.java:276) at org.apache.openjpa.kernel.AttachStrategy.attachField(AttachStrategy.java:193) at org.apache.openjpa.kernel.VersionAttachStrategy.attach(VersionAttachStrategy.java:134) at org.apache.openjpa.kernel.AttachManager.attach(AttachManager.java:239) at org.apache.openjpa.kernel.AttachStrategy.attachCollection(AttachStrategy.java:330) at org.apache.openjpa.kernel.AttachStrategy.replaceCollection(AttachStrategy.java:298) at org.apache.openjpa.kernel.AttachStrategy.attachField(AttachStrategy.java:220) at org.apache.openjpa.kernel.VersionAttachStrategy.attach(VersionAttachStrategy.java:134) at org.apache.openjpa.kernel.AttachManager.attach(AttachManager.java:239) at org.apache.openjpa.kernel.AttachStrategy.attachCollection(AttachStrategy.java:330) at org.apache.openjpa.kernel.DetachedStateManager.attach(DetachedStateManager.java:250) at org.apache.openjpa.kernel.AttachManager.attach(AttachManager.java:239) at org.apache.openjpa.kernel.AttachManager.attach(AttachManager.java:100) at org.apache.openjpa.kernel.BrokerImpl.attach(BrokerImpl.java:3176) at org.apache.openjpa.kernel.DelegatingBroker.attach(DelegatingBroker.java:1147) at org.apache.openjpa.persistence.EntityManagerImpl.merge(EntityManagerImpl.java:665) at org.piercecountywa.personnel.openjpa.TestMultigenOneToManyMerge.testMergeDetached(TestMultigenOneToManyMerge.java:108) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source) at java.lang.reflect.Method.invoke(Unknown Source) at junit.framework.TestCase.runTest(TestCase.java:164) at junit.framework.TestCase.runBare(TestCase.java:130) at junit.framework.TestResult$1.protect(TestResult.java:110) at junit.framework.TestResult.runProtected(TestResult.java:128) at junit.framework.TestResult.run(TestResult.java:113) at junit.framework.TestCase.run(TestCase.java:120) at junit.framework.TestSuite.runTest(TestSuite.java:228) at junit.framework.TestSuite.run(TestSuite.java:223) at org.junit.internal.runners.OldTestClassRunner.run(OldTestClassRunner.java:35) at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:38) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:460) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:673) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:386) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:196) Again, the good news is the first test case runs great and what you're trying to do in the second test should work great too. If you can figure out what the issue is on that second test please post it.
          Hide
          Gokhan Ergul added a comment -

          Reece --thanks for trying them out, good news indeed on the attached-merge front. The issue with detached testcase is as follows: merge A cascades to B, which in turn cascades to C, since the relations between A -> B and B -> C are annotated with Cascade.ALL on OneToMany sides. But as there's no cascade annotation on ManyToOne side (owner side of bidirectional relation), when AttachManager tries to attach those fields (such as C.parent), it encounters a new object (an instance of EntityB), and complains as such. In all fairness, that's correct – B is a new object and there's no Cascade.MERGE from C.parent to B. The missing piece of information is, that B object has already been attached (evidently since manager._attached map contains it) as part of that "merge session" so to speak --that's the object via which AttachManager got to object C in the first place, and B will be persisted as part of the same transaction. So there's no grounds to reject it. That's what the patch I attached along with the testcase does. If you could go ahead and apply it, you should no longer see that error.

          Show
          Gokhan Ergul added a comment - Reece --thanks for trying them out, good news indeed on the attached-merge front. The issue with detached testcase is as follows: merge A cascades to B, which in turn cascades to C, since the relations between A -> B and B -> C are annotated with Cascade.ALL on OneToMany sides. But as there's no cascade annotation on ManyToOne side (owner side of bidirectional relation), when AttachManager tries to attach those fields (such as C.parent), it encounters a new object (an instance of EntityB), and complains as such. In all fairness, that's correct – B is a new object and there's no Cascade.MERGE from C.parent to B. The missing piece of information is, that B object has already been attached (evidently since manager._attached map contains it) as part of that "merge session" so to speak --that's the object via which AttachManager got to object C in the first place, and B will be persisted as part of the same transaction. So there's no grounds to reject it. That's what the patch I attached along with the testcase does. If you could go ahead and apply it, you should no longer see that error.
          Hide
          Markus Fuchs added a comment -

          Hi Reece,

          I ran the JPA TCK with your changes, and all tests passed!
          Congratulations.

          But I'd need some more time to really understand your changes. One
          question upfront: What does "dereference" mean? Does it mean changing
          the reference to a persistence capable object?

          Hi Reece & Gokhan:

          Are non-nullable foreign keys the only reason for constraint
          dependencies? One update can be dependent on another update in case of
          a OneToOne relationship, as OneToOne relationships are usually
          enforced by unique keys in the database. Please also consider the
          following situation:

          a -> b;
          c -> d;

          tx.begin();
          c -> b;
          a -> null;
          tx.commit();

          B/c of the unique key, the foreign key in the row corresponding to "a"
          has to be nullified, before the foreign key in row "c" can be
          updated. Does this dependency also need to be addressed?

          Is above dependency different from the dependencies imposed by
          non-nullable foreign keys? I.e. can non-nullable foreign keys cause
          dependencies, that can't be addressed by the user order of operations?
          Are non-nullable foreign keys different in this regard?

          Thanks and sorry for my late reply!

          – markus.

          Show
          Markus Fuchs added a comment - Hi Reece, I ran the JPA TCK with your changes, and all tests passed! Congratulations. But I'd need some more time to really understand your changes. One question upfront: What does "dereference" mean? Does it mean changing the reference to a persistence capable object? Hi Reece & Gokhan: Are non-nullable foreign keys the only reason for constraint dependencies? One update can be dependent on another update in case of a OneToOne relationship, as OneToOne relationships are usually enforced by unique keys in the database. Please also consider the following situation: a -> b; c -> d; tx.begin(); c -> b; a -> null; tx.commit(); B/c of the unique key, the foreign key in the row corresponding to "a" has to be nullified, before the foreign key in row "c" can be updated. Does this dependency also need to be addressed? Is above dependency different from the dependencies imposed by non-nullable foreign keys? I.e. can non-nullable foreign keys cause dependencies, that can't be addressed by the user order of operations? Are non-nullable foreign keys different in this regard? Thanks and sorry for my late reply! – markus.
          Hide
          Gokhan Ergul added a comment -

          Marcus,

          I'm not familiar with the specifics of Reece's patch (yet), but to answer your question from a theoretical point of view: As your example demonstrates, not-nullable FKs are not the only reason for such dependencies. The real issue with ordering statements is not about nullable/not-nullable constraints, but rather with whether the target database supports deferred constraints or not (constraints enforced at transaction commit time, as opposed constraints enforced after each statement). If the target database supports deferred constraints, we'd have no need to reorder statements --assuming that resulting set of rows (or lack of them) at transaction commit do not violate any constraints. Otherwise the transaction will fail anyway, irrespective of if/how you order the statements.

          On a related note, since you have access to TCK tests, is there any chance you can apply the patch I posted to see if it breaks anything? If we could take that detached testcase out of the picture I'll go ahead mark OPENJPA-231 resolved, since the issue with attached testcase is the same with this one.

          Thanks,

          Gokhan.

          Show
          Gokhan Ergul added a comment - Marcus, I'm not familiar with the specifics of Reece's patch (yet), but to answer your question from a theoretical point of view: As your example demonstrates, not-nullable FKs are not the only reason for such dependencies. The real issue with ordering statements is not about nullable/not-nullable constraints, but rather with whether the target database supports deferred constraints or not (constraints enforced at transaction commit time, as opposed constraints enforced after each statement). If the target database supports deferred constraints, we'd have no need to reorder statements --assuming that resulting set of rows (or lack of them) at transaction commit do not violate any constraints. Otherwise the transaction will fail anyway, irrespective of if/how you order the statements. On a related note, since you have access to TCK tests, is there any chance you can apply the patch I posted to see if it breaks anything? If we could take that detached testcase out of the picture I'll go ahead mark OPENJPA-231 resolved, since the issue with attached testcase is the same with this one. Thanks, Gokhan.
          Hide
          Reece Garrett added a comment -

          Markus,

          That is great news! Thanks for running those test and getting back to me.

          Dereferenced means that a persistence capable object was either nulled or replaced. I need to capture this information because the dereference may cause a delete. If that is the case, then by the time the code gets to OperationOrderUpdateManager there is no way to calculated the dependency between the delete and the update which caused it without knowing who dereferenced who.

          As a concrete example: A has a non-nullable reference to B and no one else refers to B. You replace B with B2 thus dereferencing B. B is now orphaned and will be deleted. You get three rows generated:

          Row 1: insert B2

          Row 2: update A to point to B2

          Row 3: delete B

          So, row 2 depends on row 1 and row 3 depends on row 2. But, without knowing that the state manager for row 2 was responsible for dereferencing the state manager for row 3 (thus causing the delete) there is no way to know that row 3 depends on row 2. Without having that dependency row 3 could be run before row 2 and would violate the foreign key constraint.

          As for your second question, no, non-nullable foreign keys are not the only reason for constraint
          dependencies. Unique key constraints also cause dependencies. With unique keys an insert can depend on an update or delete and an update can depend on another update or delete. I mentioned earlier that I have a plan for dealing with this but decided that it would be best complete my patch for foreign keys first and then tackle uniques. I have a post on the mailing list where I get into a bit more detail. The post was placed on May 22nd and is titled "SQL ordering and unique constraints".

          Hope that all made sense. If you have any more questions please ask away.

          Gokhan,

          I'm sorry but I have not had a chance to apply your patch and try my code with it yet. I'll try my best to get to it but assuming your patch corrects the problem I was running into I see no reason why it won't work. I'm being lazy by not looking but have you posted your issue as a bug and posted the patch for these guys to test?

          -Reece

          Show
          Reece Garrett added a comment - Markus, That is great news! Thanks for running those test and getting back to me. Dereferenced means that a persistence capable object was either nulled or replaced. I need to capture this information because the dereference may cause a delete. If that is the case, then by the time the code gets to OperationOrderUpdateManager there is no way to calculated the dependency between the delete and the update which caused it without knowing who dereferenced who. As a concrete example: A has a non-nullable reference to B and no one else refers to B. You replace B with B2 thus dereferencing B. B is now orphaned and will be deleted. You get three rows generated: Row 1: insert B2 Row 2: update A to point to B2 Row 3: delete B So, row 2 depends on row 1 and row 3 depends on row 2. But, without knowing that the state manager for row 2 was responsible for dereferencing the state manager for row 3 (thus causing the delete) there is no way to know that row 3 depends on row 2. Without having that dependency row 3 could be run before row 2 and would violate the foreign key constraint. As for your second question, no, non-nullable foreign keys are not the only reason for constraint dependencies. Unique key constraints also cause dependencies. With unique keys an insert can depend on an update or delete and an update can depend on another update or delete. I mentioned earlier that I have a plan for dealing with this but decided that it would be best complete my patch for foreign keys first and then tackle uniques. I have a post on the mailing list where I get into a bit more detail. The post was placed on May 22nd and is titled "SQL ordering and unique constraints". Hope that all made sense. If you have any more questions please ask away. Gokhan, I'm sorry but I have not had a chance to apply your patch and try my code with it yet. I'll try my best to get to it but assuming your patch corrects the problem I was running into I see no reason why it won't work. I'm being lazy by not looking but have you posted your issue as a bug and posted the patch for these guys to test? -Reece
          Hide
          Markus Fuchs added a comment -

          Gokhan,

          I applied your patch and ran the TCK successfully again. Your testcase TestMultigenOneToManyMerge passes, too.

          – markus.

          Show
          Markus Fuchs added a comment - Gokhan, I applied your patch and ran the TCK successfully again. Your testcase TestMultigenOneToManyMerge passes, too. – markus.
          Hide
          Gokhan Ergul added a comment -

          Markus,

          Good news, thanks for your time and effort. What's the process to get the patch and testcases reviewed/committed?

          Reece --yep, it's been there for a while (OPENJPA-231), didn't get as much attention as this one though.

          Show
          Gokhan Ergul added a comment - Markus, Good news, thanks for your time and effort. What's the process to get the patch and testcases reviewed/committed? Reece --yep, it's been there for a while ( OPENJPA-231 ), didn't get as much attention as this one though.
          Hide
          Reece Garrett added a comment -

          Is there any update on the status of my patch? I don't want to start work on a new patch to deal with unique key constraints until this patch gets committed. I'm sure you've all been busy what with OpenJPA being promoted to a top level project.

          -Reece

          Show
          Reece Garrett added a comment - Is there any update on the status of my patch? I don't want to start work on a new patch to deal with unique key constraints until this patch gets committed. I'm sure you've all been busy what with OpenJPA being promoted to a top level project. -Reece
          Hide
          Patrick Linskey added a comment -

          Hi,

          I just checked in a change (r544918) that does re-ordering according to a graph traversal. It looks like your solution has better handling of nulled relations than the code that I checked in, so we might want to look at how to integrate the two approaches a bit more. I haven't tried your tests against these changes, though. Ideally, it'd be good to get your test cases into a patch form that we can just check into the codebase directly.

          Show
          Patrick Linskey added a comment - Hi, I just checked in a change (r544918) that does re-ordering according to a graph traversal. It looks like your solution has better handling of nulled relations than the code that I checked in, so we might want to look at how to integrate the two approaches a bit more. I haven't tried your tests against these changes, though. Ideally, it'd be good to get your test cases into a patch form that we can just check into the codebase directly.
          Hide
          Markus Fuchs added a comment -

          Hi Patrick,

          from comments in ConstraintUpdateManager (line 101,102) I got the impression, that you assume that inserts can not be dependent on updates. The attached testcase displays a situation (involving an unique key) where an insert is actually dependent on an update.

          Best regards,

          – markus.

          Show
          Markus Fuchs added a comment - Hi Patrick, from comments in ConstraintUpdateManager (line 101,102) I got the impression, that you assume that inserts can not be dependent on updates. The attached testcase displays a situation (involving an unique key) where an insert is actually dependent on an update. Best regards, – markus.
          Markus Fuchs made changes -
          Attachment openjpa-235-test2.zip [ 12359131 ]
          Hide
          Reece Garrett added a comment -

          Hello Patrick,

          Your patch works great for foreign keys but I wrote my patch in anticipation of adding uniqueness constraint dependencies. Markus is correct, when you consider uniqueness constraints an insert can depend on an update or a delete and an update can depend on another update or a delete.

          Say you have an insert which depends on a delete because of a uniqueness constraint. The delete should get ordered first. However, the delete was originally generated because there is an update either nulling or replacing a foreign key reference to it . If the delete runs before the update a foreign key violation will occur because the row being deleted has not been dereferenced by the update yet. Upon seeing that the delete precedes the update which it depends on the code must update the offending foreign key to null before the delete executes (this assumes the foreign key is nullable, if not then you're out of luck).

          The code you committed does not know about the dependency between the delete and the update and thus can not handle the above situation. In fact, without having my list of dereferenced states for each state manager I do not see how the dependency can be calculated.

          Other than that, the code looks great. My test cases pass and I will attach them as a patch ("sqlReorderTests.patch").

          Any input you have on the uniqueness constraint issue would be much appreciated because that's the last piece to this puzzle. Secretly I'm hoping you guys will tackle this one because it doesn't look too hard and will give you guys (another) advantage over the competition. I know I can do it but it will require going a bit deeper into the code than I feel totally comfortable with. For all updates and deletes I'd have to store the original values (as they stood at the beginning of the transaction) for columns involved in uniqueness constraints. Then, when calculating dependencies for inserts and updates where uniqueness constraints are involved I'd check all updates and deletes going to the same table and see if they need to be run first to prevent a unique key violation. Like I said, it shouldn't be rocket science as all the data needed to do this is pulled from the database; just not stored for later access. Updates have some of these original values stored for rollback but deletes don't store any of the original values except the primary key and version.

          Basically, if there was a method "RowImpl.getOriginal(Column col)" that works just like the existing method "RowImpl.getSet(Column col)" but returns the original value for the specified column then it would be smooth sailing from there.

          -Reece

          Show
          Reece Garrett added a comment - Hello Patrick, Your patch works great for foreign keys but I wrote my patch in anticipation of adding uniqueness constraint dependencies. Markus is correct, when you consider uniqueness constraints an insert can depend on an update or a delete and an update can depend on another update or a delete. Say you have an insert which depends on a delete because of a uniqueness constraint. The delete should get ordered first. However, the delete was originally generated because there is an update either nulling or replacing a foreign key reference to it . If the delete runs before the update a foreign key violation will occur because the row being deleted has not been dereferenced by the update yet. Upon seeing that the delete precedes the update which it depends on the code must update the offending foreign key to null before the delete executes (this assumes the foreign key is nullable, if not then you're out of luck). The code you committed does not know about the dependency between the delete and the update and thus can not handle the above situation. In fact, without having my list of dereferenced states for each state manager I do not see how the dependency can be calculated. Other than that, the code looks great. My test cases pass and I will attach them as a patch ("sqlReorderTests.patch"). Any input you have on the uniqueness constraint issue would be much appreciated because that's the last piece to this puzzle. Secretly I'm hoping you guys will tackle this one because it doesn't look too hard and will give you guys (another) advantage over the competition. I know I can do it but it will require going a bit deeper into the code than I feel totally comfortable with. For all updates and deletes I'd have to store the original values (as they stood at the beginning of the transaction) for columns involved in uniqueness constraints. Then, when calculating dependencies for inserts and updates where uniqueness constraints are involved I'd check all updates and deletes going to the same table and see if they need to be run first to prevent a unique key violation. Like I said, it shouldn't be rocket science as all the data needed to do this is pulled from the database; just not stored for later access. Updates have some of these original values stored for rollback but deletes don't store any of the original values except the primary key and version. Basically, if there was a method "RowImpl.getOriginal(Column col)" that works just like the existing method "RowImpl.getSet(Column col)" but returns the original value for the specified column then it would be smooth sailing from there. -Reece
          Reece Garrett made changes -
          Attachment sqlReorderTests.patch [ 12359216 ]
          Pinaki Poddar made changes -
          Link This issue incorporates OPENJPA-253 [ OPENJPA-253 ]
          Hide
          Patrick Linskey added a comment -

          > Basically, if there was a method "RowImpl.getOriginal(Column col)" that
          > works just like the existing method "RowImpl.getSet(Column col)" but
          > returns the original value for the specified column then it would be smooth
          > sailing from there.

          Would it be sufficient to just always run the deletes first, then the updates, and finally the inserts, when there are unique constraints involved? Assuming that we optimize away any SQL operations for insert-then-delete-before-flush, it would seem that that algorithm would be guaranteed to work when no constraint violations were generated in the business code, which I think is all that we can really strive for in the case of unique constraints.

          Show
          Patrick Linskey added a comment - > Basically, if there was a method "RowImpl.getOriginal(Column col)" that > works just like the existing method "RowImpl.getSet(Column col)" but > returns the original value for the specified column then it would be smooth > sailing from there. Would it be sufficient to just always run the deletes first, then the updates, and finally the inserts, when there are unique constraints involved? Assuming that we optimize away any SQL operations for insert-then-delete-before-flush, it would seem that that algorithm would be guaranteed to work when no constraint violations were generated in the business code, which I think is all that we can really strive for in the case of unique constraints.
          Hide
          Craig L Russell added a comment -

          > Would it be sufficient to just always run the deletes first, then the updates, and finally the inserts, when there are unique constraints involved? Assuming that we optimize away any SQL operations for insert-then-delete-before-flush, it would seem that that algorithm would be guaranteed to work when no constraint violations were generated in the business code, which I think is all that we can really strive for in the case of unique constraints.

          This algorithm isn't quite enough. Consider deleting an instance that has a reference from an instance that is being updated to nullify that reference. Then you want to do the update to nullify the reference before you do the delete.

          I think the general issue is that due to uniqueness and referential integrity constraints you need to create a dependency graph and then order the operations to preserve the dependencies. But you also want to preserve the order the user made the changes to avoid deadlocks due to locking dependencies. And you also want to reorder operations to take advantage of statement batching.

          Show
          Craig L Russell added a comment - > Would it be sufficient to just always run the deletes first, then the updates, and finally the inserts, when there are unique constraints involved? Assuming that we optimize away any SQL operations for insert-then-delete-before-flush, it would seem that that algorithm would be guaranteed to work when no constraint violations were generated in the business code, which I think is all that we can really strive for in the case of unique constraints. This algorithm isn't quite enough. Consider deleting an instance that has a reference from an instance that is being updated to nullify that reference. Then you want to do the update to nullify the reference before you do the delete. I think the general issue is that due to uniqueness and referential integrity constraints you need to create a dependency graph and then order the operations to preserve the dependencies. But you also want to preserve the order the user made the changes to avoid deadlocks due to locking dependencies. And you also want to reorder operations to take advantage of statement batching.
          Hide
          Patrick Linskey added a comment -

          > This algorithm isn't quite enough. Consider deleting an instance that has
          > a reference from an instance that is being updated to nullify that
          > reference. Then you want to do the update to nullify the reference before
          > you do the delete.

          That should be already caught in the graph traversal that's in place, since presumably such a constraint would be a foreign key constraint, not a unique constraint. Am I missing something?

          Show
          Patrick Linskey added a comment - > This algorithm isn't quite enough. Consider deleting an instance that has > a reference from an instance that is being updated to nullify that > reference. Then you want to do the update to nullify the reference before > you do the delete. That should be already caught in the graph traversal that's in place, since presumably such a constraint would be a foreign key constraint, not a unique constraint. Am I missing something?
          Hide
          Reece Garrett added a comment -

          > Would it be sufficient to just always run the deletes first, then the updates, and finally the inserts, when there are unique constraints involved?
          > Assuming that we optimize away any SQL operations for insert-then-delete-before-flush, it would seem that that algorithm would be guaranteed to > work when no constraint violations were generated in the business code, which I think is all that we can really strive for in the case of unique
          > constraints.

          That won't work because updates can have inner dependencies due to uniqueness constraints. For instance, you have a table with a unique column and two rows. The first row has value '1' in the unique column and the second row has value '2' in the unique column. You want to update the unique column in the first row to value '2' and the unique column in the second row to value '3'. The second row must be updated first but the only way to know that is to have access to the original values of both rows. I see no way to properly deal with unique keys without snapshots of the original rows.

          Like I said, OpenJPA already pulls the origin values and if I could access them with a method like RowImpl.getOriginal(Column col) then I could modify my patch to deal with both unique and foreign key constraints in no time flat. The original values could be put in an array on the row the same way set values for updates are put in the _vals array.

          Also, although blindly putting deletes before inserts and updates works for uniqueness constraints, it does not work for foreign key constraints because it is possible for either an update or another delete to depend on that delete. Putting the delete first only works if all inbound foreign key dependencies are nulled first. But, there are two problems with that.

          1) The patch you committed does not know about dependencies between updates and the deletes they have caused by breaking relationships to other objects. You need to know that if you're planning on arbitrarily ordering deletes first for uniqueness constraints, otherwise, you won't know that an update needs to be generated to null a foreign key which relies on the row your about to delete. That's why I store dereferenced states in my patch. By the time your code executes that information is lost in space.

          2) By ordering a delete first for tables with uniqueness constraints without checking to see if an insert or update actually depends on it (only way is to check the original values) you could be generating unnecessary updates to null foreign keys.

          In a nutshell, if we want to do unique and foreign key constraints properly we need original values on both updates and deletes and be able to access those values using the columns they map to, just like is done for RowImpl.getSet(Column col). And we need to be able to calculate dependencies between deletes and updates in cases where the delete was caused by the update.

          -Reece

          Show
          Reece Garrett added a comment - > Would it be sufficient to just always run the deletes first, then the updates, and finally the inserts, when there are unique constraints involved? > Assuming that we optimize away any SQL operations for insert-then-delete-before-flush, it would seem that that algorithm would be guaranteed to > work when no constraint violations were generated in the business code, which I think is all that we can really strive for in the case of unique > constraints. That won't work because updates can have inner dependencies due to uniqueness constraints. For instance, you have a table with a unique column and two rows. The first row has value '1' in the unique column and the second row has value '2' in the unique column. You want to update the unique column in the first row to value '2' and the unique column in the second row to value '3'. The second row must be updated first but the only way to know that is to have access to the original values of both rows. I see no way to properly deal with unique keys without snapshots of the original rows. Like I said, OpenJPA already pulls the origin values and if I could access them with a method like RowImpl.getOriginal(Column col) then I could modify my patch to deal with both unique and foreign key constraints in no time flat. The original values could be put in an array on the row the same way set values for updates are put in the _vals array. Also, although blindly putting deletes before inserts and updates works for uniqueness constraints, it does not work for foreign key constraints because it is possible for either an update or another delete to depend on that delete. Putting the delete first only works if all inbound foreign key dependencies are nulled first. But, there are two problems with that. 1) The patch you committed does not know about dependencies between updates and the deletes they have caused by breaking relationships to other objects. You need to know that if you're planning on arbitrarily ordering deletes first for uniqueness constraints, otherwise, you won't know that an update needs to be generated to null a foreign key which relies on the row your about to delete. That's why I store dereferenced states in my patch. By the time your code executes that information is lost in space. 2) By ordering a delete first for tables with uniqueness constraints without checking to see if an insert or update actually depends on it (only way is to check the original values) you could be generating unnecessary updates to null foreign keys. In a nutshell, if we want to do unique and foreign key constraints properly we need original values on both updates and deletes and be able to access those values using the columns they map to, just like is done for RowImpl.getSet(Column col). And we need to be able to calculate dependencies between deletes and updates in cases where the delete was caused by the update. -Reece
          Hide
          Markus Fuchs added a comment -

          Hi all,

          I propose the following steps to extend dependency management based on
          the current implementation in ConstraintUpdateManager:

          The current dependency calculation is based on a Depth First Search
          (DFS) algorithm on the dependency graph defined by foreign key
          constraints for inserts and deletes. Unique key constraints are
          currently not handled.

          1) Circles are identified by "BACK" edges found by the DFS and are
          broken right there by nullifying the foreign key corresponding to
          the "BACK" edge. This doesn't work, if this foreign key is not
          nullable. Instead, I propose to identify the edges that contribute
          to the circle in the NodeInfo corresponding to each graph node. If
          we have the info about about the edges on the circle, we can walk
          the circle and break it at the first nullable foreign key
          found. Note, that there can't be a circle consisting of only
          non-nullable foreign keys!

          2) In a second phase, incorporate dependencies based on unique keys
          into the graph construction. That means that instead of possibly
          getting two independent graphs, one for inserts and one for
          deletes, we'd always have just one graph including inserts,
          updates, and deletes. The flush algorithm described in 1) would
          stay the same, just the dependency graph would have additional
          edges.

          3) Independent rows (rows that weren't included in the dependency
          graph) are currently flushed in random order. This might cause
          deadlocks in the database. As an additional feature, we might want
          to flush these rows in the order they were modified in the
          transaction.

          Does this make sense? What do you think? Did I miss anything?

          Thanks,

          – markus.

          Show
          Markus Fuchs added a comment - Hi all, I propose the following steps to extend dependency management based on the current implementation in ConstraintUpdateManager: The current dependency calculation is based on a Depth First Search (DFS) algorithm on the dependency graph defined by foreign key constraints for inserts and deletes. Unique key constraints are currently not handled. 1) Circles are identified by "BACK" edges found by the DFS and are broken right there by nullifying the foreign key corresponding to the "BACK" edge. This doesn't work, if this foreign key is not nullable. Instead, I propose to identify the edges that contribute to the circle in the NodeInfo corresponding to each graph node. If we have the info about about the edges on the circle, we can walk the circle and break it at the first nullable foreign key found. Note, that there can't be a circle consisting of only non-nullable foreign keys! 2) In a second phase, incorporate dependencies based on unique keys into the graph construction. That means that instead of possibly getting two independent graphs, one for inserts and one for deletes, we'd always have just one graph including inserts, updates, and deletes. The flush algorithm described in 1) would stay the same, just the dependency graph would have additional edges. 3) Independent rows (rows that weren't included in the dependency graph) are currently flushed in random order. This might cause deadlocks in the database. As an additional feature, we might want to flush these rows in the order they were modified in the transaction. Does this make sense? What do you think? Did I miss anything? Thanks, – markus.
          Hide
          Reece Garrett added a comment -

          Markus,

          That looks like an excellent solution. It doesn't look like you've missed anything.

          -Reece

          Show
          Reece Garrett added a comment - Markus, That looks like an excellent solution. It doesn't look like you've missed anything. -Reece
          Hide
          Reece Garrett added a comment -

          Hello all,

          I would like to know if anyone has plans or is in the process of implementing Markus's solution? Obviously, based on the interest this has generated, it is an important addition to OpenJPA. However, for the moment I can not devote anymore development hours to working on a new patch. Right now we are dealing with unique keys by...not using them. This will work in the short term but we'll have to find a way to handle them soon. Perhaps at that point I'll get more hours to work on a patch but I'm a bit apprehensive because as I mentioned earlier, it will require going a bit deeper in to the code to store original values to calculate unique key dependencies. It is also difficult working on this without access to the TCK tests.

          As Markus suggests, the solution is basically the current implementation in ConstraintUpdateManager with provisions for capturing dereferenced rows to calculate update-delete dependencies (my second patch does this), circular reference handling (my second patch does minimal handling but Markus's solution is better), deadlock prevention, and of course unique key dependencies.

          -Reece

          Show
          Reece Garrett added a comment - Hello all, I would like to know if anyone has plans or is in the process of implementing Markus's solution? Obviously, based on the interest this has generated, it is an important addition to OpenJPA. However, for the moment I can not devote anymore development hours to working on a new patch. Right now we are dealing with unique keys by...not using them. This will work in the short term but we'll have to find a way to handle them soon. Perhaps at that point I'll get more hours to work on a patch but I'm a bit apprehensive because as I mentioned earlier, it will require going a bit deeper in to the code to store original values to calculate unique key dependencies. It is also difficult working on this without access to the TCK tests. As Markus suggests, the solution is basically the current implementation in ConstraintUpdateManager with provisions for capturing dereferenced rows to calculate update-delete dependencies (my second patch does this), circular reference handling (my second patch does minimal handling but Markus's solution is better), deadlock prevention, and of course unique key dependencies. -Reece
          Hide
          Markus Fuchs added a comment -

          Hi Reece,

          I started implementing my proposal. As the 3 steps are pretty much independent, I'll be posting patches and new test cases for every phase here. I'm looking forward to using some of the code you already provided. Thanks for getting this started.

          – markus.

          Show
          Markus Fuchs added a comment - Hi Reece, I started implementing my proposal. As the 3 steps are pretty much independent, I'll be posting patches and new test cases for every phase here. I'm looking forward to using some of the code you already provided. Thanks for getting this started. – markus.
          Hide
          Reece Garrett added a comment -

          Markus,

          Excellent, we're in good hands then. Please let me know if you have any questions about my code or in general.

          -Reece

          Show
          Reece Garrett added a comment - Markus, Excellent, we're in good hands then. Please let me know if you have any questions about my code or in general. -Reece
          Hide
          Kevin Sutter added a comment -

          Can someone summarize the current state of this JIRA report? From the comments and the commits, it sounds like Patrick has committed at least a partial solution, but there are still some holes based on the original JIRA report. I have not seen another patch posted after Patrick's commits on June 06 and 07, except for testcase patches.

          So, what are we waiting on? Do we have additional code patches or commits required to finish up this report? It sounds like Markus is working on an implementation, but is to resolve this Issue or a new proposal? What about the testcases? Patrick integrated some changes for the testcases, but are they complete?

          Thanks,
          Kevin

          Show
          Kevin Sutter added a comment - Can someone summarize the current state of this JIRA report? From the comments and the commits, it sounds like Patrick has committed at least a partial solution, but there are still some holes based on the original JIRA report. I have not seen another patch posted after Patrick's commits on June 06 and 07, except for testcase patches. So, what are we waiting on? Do we have additional code patches or commits required to finish up this report? It sounds like Markus is working on an implementation, but is to resolve this Issue or a new proposal? What about the testcases? Patrick integrated some changes for the testcases, but are they complete? Thanks, Kevin
          Hide
          Kevan Miller added a comment -

          Geronimo testing has started running into duplicate key and foreign key constraint exceptions. I've tracked the problem down to revision 544918.

          I haven't, however, fully diagnosed the problem... Are you not seeing any problems in your testing?

          BTW, Geronimo has a policy which allows interested Apache committers from other projects to gain guest access to our tck infrastructure. This has worked really well with Axis and CXF projects. Would be great to have OpenJPA participation, also. If anyone's interested contact me or our dev list...

          Show
          Kevan Miller added a comment - Geronimo testing has started running into duplicate key and foreign key constraint exceptions. I've tracked the problem down to revision 544918. I haven't, however, fully diagnosed the problem... Are you not seeing any problems in your testing? BTW, Geronimo has a policy which allows interested Apache committers from other projects to gain guest access to our tck infrastructure. This has worked really well with Axis and CXF projects. Would be great to have OpenJPA participation, also. If anyone's interested contact me or our dev list...
          Hide
          Patrick Linskey added a comment -

          That's surprising, since OpenJPA passes the JPA TCK with the change. Do you have any more details about which test is failing?

          Also, note that you can always manually specify the old operation-order update manager (via the openjpa.jdbc.UpdateManager property), which will give you the old behavior.

          Show
          Patrick Linskey added a comment - That's surprising, since OpenJPA passes the JPA TCK with the change. Do you have any more details about which test is failing? Also, note that you can always manually specify the old operation-order update manager (via the openjpa.jdbc.UpdateManager property), which will give you the old behavior.
          Hide
          Reece Garrett added a comment -

          Kevin,

          The patch which Patrick submitted is not a complete solution, however, it seems to work quite well for foreign key constraints. I'm not certain what you mean by "complete", but the test cases I submitted exercise the code in such a way that a foreign key constraint violation should occur if ordering is not done properly. Markus is in the process of implementing the changes he outlined a few posts back so we're waiting for him to contribute his code.

          Kevan,

          Revision 544918 is not my code and to be honest I haven't had a chance to look at it in depth. I did write the test cases that were committed and they all pass; perhaps there are scenarios I failed to address. Without knowing the specifics of the exceptions it's hard to say what the problem could be. Please let us know when you determine the exact cause.

          I am definitely interested in gaining access to your tck infrastructure. As I said earlier, I work for a government organization so signing the NDA presents certain legal issues.

          -Reece

          Show
          Reece Garrett added a comment - Kevin, The patch which Patrick submitted is not a complete solution, however, it seems to work quite well for foreign key constraints. I'm not certain what you mean by "complete", but the test cases I submitted exercise the code in such a way that a foreign key constraint violation should occur if ordering is not done properly. Markus is in the process of implementing the changes he outlined a few posts back so we're waiting for him to contribute his code. Kevan, Revision 544918 is not my code and to be honest I haven't had a chance to look at it in depth. I did write the test cases that were committed and they all pass; perhaps there are scenarios I failed to address. Without knowing the specifics of the exceptions it's hard to say what the problem could be. Please let us know when you determine the exact cause. I am definitely interested in gaining access to your tck infrastructure. As I said earlier, I work for a government organization so signing the NDA presents certain legal issues. -Reece
          Hide
          Markus Fuchs added a comment -

          Hi Kevin & Kevan,

          The current implementation in ConstraintUpdateManager handles foreign key constraints correctly and resolves circular dependencies. AFAIK, the only pieces missing are:

          1) OpenJPA might try to nullify a non-nullable foreign key in case of circular dependencies
          2) Dependencies based on unique keys and not handled right now

          I'm working on add these features as described in my earlier post.

          – markus.

          Show
          Markus Fuchs added a comment - Hi Kevin & Kevan, The current implementation in ConstraintUpdateManager handles foreign key constraints correctly and resolves circular dependencies. AFAIK, the only pieces missing are: 1) OpenJPA might try to nullify a non-nullable foreign key in case of circular dependencies 2) Dependencies based on unique keys and not handled right now I'm working on add these features as described in my earlier post. – markus.
          Hide
          Kevan Miller added a comment -

          Reece, afraid access to the tck would still require signing the NDA.

          Patrick, afraid I can't really discuss details of the test failures. They wouldn't be in a "JPA" test suite. This is why it would be great if we could share some of this information more openly. Best (only?) way that I know of is by getting access to our TCK.

          I'll see if I (or someon else) can generate enough info on the problem...

          Show
          Kevan Miller added a comment - Reece, afraid access to the tck would still require signing the NDA. Patrick, afraid I can't really discuss details of the test failures. They wouldn't be in a "JPA" test suite. This is why it would be great if we could share some of this information more openly. Best (only?) way that I know of is by getting access to our TCK. I'll see if I (or someon else) can generate enough info on the problem...
          Hide
          Reece Garrett added a comment -

          It's times like these when open source doesn't seem so.....open.

          Show
          Reece Garrett added a comment - It's times like these when open source doesn't seem so.....open.
          Hide
          Marc Prud'hommeaux added a comment -

          Kevan-

          If you set the "openjpa.jdbc.UpdateManager" to be "operation-order", do you still see the failures? That will at least help identify if this is a problem that always happened, or if it is new and is due to the FK ordering support.

          Show
          Marc Prud'hommeaux added a comment - Kevan- If you set the "openjpa.jdbc.UpdateManager" to be "operation-order", do you still see the failures? That will at least help identify if this is a problem that always happened, or if it is new and is due to the FK ordering support.
          Hide
          Kevan Miller added a comment -

          Patrick and Marc,
          Setting -Dopenjpa.jdbc.UpdateManager=operation-order does avoid the problem. Let me know if there's additional data that can be gathered...

          Reece,
          Understood. The NDA is required because of Sun restrictions (you would be getting access to Sun restricted materials). I certainly wish we didn't have to go through these shenanigans...

          Show
          Kevan Miller added a comment - Patrick and Marc, Setting -Dopenjpa.jdbc.UpdateManager=operation-order does avoid the problem. Let me know if there's additional data that can be gathered... Reece, Understood. The NDA is required because of Sun restrictions (you would be getting access to Sun restricted materials). I certainly wish we didn't have to go through these shenanigans...
          Hide
          Gokhan Ergul added a comment -

          Patrick,

          ConstraintUpdateManager breaks the testcase I previously submitted (attachment #2) while persisting initial entities with FK violation (against mysql, passes against derby) --before it actually gets around to test merge behaviour. That phase at least works fine with OperationOrderUpdateManager.

          Show
          Gokhan Ergul added a comment - Patrick, ConstraintUpdateManager breaks the testcase I previously submitted (attachment #2) while persisting initial entities with FK violation (against mysql, passes against derby) --before it actually gets around to test merge behaviour. That phase at least works fine with OperationOrderUpdateManager.
          Hide
          Reece Garrett added a comment -

          Kevan,

          Concerning the exceptions you're getting, one of my colleagues suggested (and I agree) that OpenJPA may not know about some foreign keys due to missing foreign key annotations. There are several cases where OpenJPA defines logical foreign keys but not actual foreign keys unless explicitly instructed to do so by the @ForeignKey or @ElementForeignKey annotations. I was not able to detect logical foreign keys when calculating dependencies between rows. I solved this problem by explicitly declaring the foreign keys I was missiong. Logical foreign keys worked well before the patch because the code did not need to detect the foreign keys for ordering.

          -Reece

          Show
          Reece Garrett added a comment - Kevan, Concerning the exceptions you're getting, one of my colleagues suggested (and I agree) that OpenJPA may not know about some foreign keys due to missing foreign key annotations. There are several cases where OpenJPA defines logical foreign keys but not actual foreign keys unless explicitly instructed to do so by the @ForeignKey or @ElementForeignKey annotations. I was not able to detect logical foreign keys when calculating dependencies between rows. I solved this problem by explicitly declaring the foreign keys I was missiong. Logical foreign keys worked well before the patch because the code did not need to detect the foreign keys for ordering. -Reece
          Gokhan Ergul made changes -
          Attachment merge-multigen-collection-testcase.zip [ 12358588 ]
          Hide
          Gokhan Ergul added a comment -

          Fixed wrong/missing headers in testcases.

          Show
          Gokhan Ergul added a comment - Fixed wrong/missing headers in testcases.
          Gokhan Ergul made changes -
          Attachment merge-testcases.patch [ 12360321 ]
          Hide
          Markus Fuchs added a comment -

          Hi Kevan,

          Is it possible that the tests failing with ConstraintUpdateManager update/null-out relationships before the instance is deleted? With the current implementation dependencies wouldn't be detected. Depending on the operation order, these situations work with OperationOrderUpdateManager.

          – markus.

          Show
          Markus Fuchs added a comment - Hi Kevan, Is it possible that the tests failing with ConstraintUpdateManager update/null-out relationships before the instance is deleted? With the current implementation dependencies wouldn't be detected. Depending on the operation order, these situations work with OperationOrderUpdateManager. – markus.
          Hide
          Markus Fuchs added a comment -

          This patch guarantees that circular dependencies are always broken at nullable foreign keys. Its successfully tested by new tests and running the JPA TCK. The changes are:

          ConstraintUpdateManager.java:
          =============================

          • analyzeForeignKeys, analyzeAgainstInserts:
            the direction of edges in the dependency graph has been reversed.
            A node has edges to other nodes it depends on
          • flushGraph:
            Handles circular constraints
          • if deleted row A has a ciricular fk to deleted row B,
            then use an update statement to null A's fk to B before flushing,
            and then flush
          • if inserted row A has a circular fk to updated/inserted row B,
            then null the fk in the B row object, then flush,
            and after flushing, use an update to set the fk back to A
            Depending on where circular dependencies are broken, the
            topological order of the graph nodes has to be re-calculated;
            The Lists deleteUpdates and insertUpdates are always initialized,
            since they are passed to resolceCycles
          • addDeleteUpdate, addInsertUpdate:
            refactored the code nulifying foreign keys into separate methods
          • findBreakableLink:
            finds a nullable foreign key by walking the dependency cycle.
          • recalculateDFA:
            Re-calculates the DepthFirstSearch analysis of the graph
            after some of the edges have been removed. Assertions ensure
            that the graph is cycle free.
          • resolveCycles:
            Resolves cycles by identifying a nullable foreign key, and
            then calling either addDeleteUpdate or addInsertUpdate depending
            on the operation causing the dependency

          ForeignKey.java:
          ================

          • hasNotNullColumns:
            checks for non-nullable local columns

          kernel/localizer.properties:
          ============================

          • added messages used in ConstraintUpdateManager.

          DepthFirstAnalysis.java:
          ========================

          • added localizer for messages
          • visit:
            a) takes a list of graph edges traversed while walking the graph in DFS;
            list is initially empty;
            always contains the edges walked to reach currently visited node
            b) the cycle is calculated for back- and forward edges;
            The cycle always exists for back edges part: of path + back edge;
            The cycle might exist for forward edges: Reasons for forward edges
            i) The graph is disconnected or ii) Threre's a cycle.
          • buildCycle:
            Constructs the cycle for back edges: part of DFS path + back edge
          • cycleForBackEdge:
            calculates the cycle for a back edge
          • cycleForForwardEdge:
            Once a forward edge is found, the graph is walked to find a path from
            the end of the forward edge back to it's beginning node
          • findNodeInPath:
            Finds the position of the edge starting from a node in a continuous
            list of edges.
          • NodeInfoComparator.compare
            Since the edge direction is reversed, node are sorted by finished order

          Edge.java:
          ==========

          • List cycle holds the cycle for back- and forward edges
          • cycle field is re-set in clearTraversal

          util/localizer.properties:
          ==========================

          • added messages used in DepthFirstAnalysis.

          TestDepthFirstAnalysis.java:
          ============================

          • refactored test data set up
          • adjusted testNodeSorting for reversed edge direction
          • added new graph resembling the commit dependencies in
            TestNoForeignKeyViolation#testComplexCycle to test cycle detection

          EntityB.java, EntityC.java:
          ===========================

          • reintroduced "non optional" property removed in last commit

          EntityD.java:
          =============

          • added two optional relationships to EntityA and EntityB. Both
            relationships may cause a circular dependency

          EntityE.java:
          =============

          • added class, has a nullable relationship to EntityB. This
            relationship doesn't cause a circular dependency

          TestNoForeignKeyViolation.java:
          ===============================

          • refactored test data set up
          • testSimpleCycle
            Dependency graph has exactly one cycle.
            The nodes on the cycle don't have any edges which are not on the cycle
          • testComplexCycle
            Like above. One node on the cycle has an edge which is not on the cycle
          • testComplexTwoCycles
            Dependency graph has two cycles. Otherwise like above.
          Show
          Markus Fuchs added a comment - This patch guarantees that circular dependencies are always broken at nullable foreign keys. Its successfully tested by new tests and running the JPA TCK. The changes are: ConstraintUpdateManager.java: ============================= analyzeForeignKeys, analyzeAgainstInserts: the direction of edges in the dependency graph has been reversed. A node has edges to other nodes it depends on flushGraph: Handles circular constraints if deleted row A has a ciricular fk to deleted row B, then use an update statement to null A's fk to B before flushing, and then flush if inserted row A has a circular fk to updated/inserted row B, then null the fk in the B row object, then flush, and after flushing, use an update to set the fk back to A Depending on where circular dependencies are broken, the topological order of the graph nodes has to be re-calculated; The Lists deleteUpdates and insertUpdates are always initialized, since they are passed to resolceCycles addDeleteUpdate, addInsertUpdate: refactored the code nulifying foreign keys into separate methods findBreakableLink: finds a nullable foreign key by walking the dependency cycle. recalculateDFA: Re-calculates the DepthFirstSearch analysis of the graph after some of the edges have been removed. Assertions ensure that the graph is cycle free. resolveCycles: Resolves cycles by identifying a nullable foreign key, and then calling either addDeleteUpdate or addInsertUpdate depending on the operation causing the dependency ForeignKey.java: ================ hasNotNullColumns: checks for non-nullable local columns kernel/localizer.properties: ============================ added messages used in ConstraintUpdateManager. DepthFirstAnalysis.java: ======================== added localizer for messages visit: a) takes a list of graph edges traversed while walking the graph in DFS; list is initially empty; always contains the edges walked to reach currently visited node b) the cycle is calculated for back- and forward edges; The cycle always exists for back edges part: of path + back edge; The cycle might exist for forward edges: Reasons for forward edges i) The graph is disconnected or ii) Threre's a cycle. buildCycle: Constructs the cycle for back edges: part of DFS path + back edge cycleForBackEdge: calculates the cycle for a back edge cycleForForwardEdge: Once a forward edge is found, the graph is walked to find a path from the end of the forward edge back to it's beginning node findNodeInPath: Finds the position of the edge starting from a node in a continuous list of edges. NodeInfoComparator.compare Since the edge direction is reversed, node are sorted by finished order Edge.java: ========== List cycle holds the cycle for back- and forward edges cycle field is re-set in clearTraversal util/localizer.properties: ========================== added messages used in DepthFirstAnalysis. TestDepthFirstAnalysis.java: ============================ refactored test data set up adjusted testNodeSorting for reversed edge direction added new graph resembling the commit dependencies in TestNoForeignKeyViolation#testComplexCycle to test cycle detection EntityB.java, EntityC.java: =========================== reintroduced "non optional" property removed in last commit EntityD.java: ============= added two optional relationships to EntityA and EntityB. Both relationships may cause a circular dependency EntityE.java: ============= added class, has a nullable relationship to EntityB. This relationship doesn't cause a circular dependency TestNoForeignKeyViolation.java: =============================== refactored test data set up testSimpleCycle Dependency graph has exactly one cycle. The nodes on the cycle don't have any edges which are not on the cycle testComplexCycle Like above. One node on the cycle has an edge which is not on the cycle testComplexTwoCycles Dependency graph has two cycles. Otherwise like above.
          Markus Fuchs made changes -
          Attachment openjpa-235-break-nullable.patch [ 12361530 ]
          Hide
          Craig L Russell added a comment -

          I'm not competent to evaluate the entire patch, but here are a few comments on openjpa-235-break-nullable.patch:

          First, the patch is nicely commented, follows the code conventions, and is very readable.

          1. Assert is used to detect fatal internal errors not user errors. For production code, assertions are turned off. It's not clear that recalculateDFA should use the assert keyword. If this code is executed after trying to break cycles, but due to the schema the cycle cannot be broken, then this is not an invariant and assert should not be used. On the other hand, if this code is only executed after successfully removing an edge and the assertion should never fail, then it's ok.

          2. Similarly, resolveCycles should not use assert, since it's a user condition not an invariant.

          3. It's hard to tell if cycleForBackEdge should use assert.

          4. The new messages in localizer.properties are pretty terse. Are these user messages or internal diagnostics? If user messages, they could be expanded. For example, instead of
          +no-nullable-fk: No nullable foreign key found to resolve circular commit dependency.
          perhaps something that tells the user
          +no-nullable-fk: No nullable foreign key found to resolve circular flush dependency. During flush processing, changes to instances, new instances, and deleted instances must be processed in a specific sequence to avoid foreign key constraint violations. The changes required in this transaction cannot be reordered because none of the foreign key constraints is nullable (optional).

          Show
          Craig L Russell added a comment - I'm not competent to evaluate the entire patch, but here are a few comments on openjpa-235-break-nullable.patch: First, the patch is nicely commented, follows the code conventions, and is very readable. 1. Assert is used to detect fatal internal errors not user errors. For production code, assertions are turned off. It's not clear that recalculateDFA should use the assert keyword. If this code is executed after trying to break cycles, but due to the schema the cycle cannot be broken, then this is not an invariant and assert should not be used. On the other hand, if this code is only executed after successfully removing an edge and the assertion should never fail, then it's ok. 2. Similarly, resolveCycles should not use assert, since it's a user condition not an invariant. 3. It's hard to tell if cycleForBackEdge should use assert. 4. The new messages in localizer.properties are pretty terse. Are these user messages or internal diagnostics? If user messages, they could be expanded. For example, instead of +no-nullable-fk: No nullable foreign key found to resolve circular commit dependency. perhaps something that tells the user +no-nullable-fk: No nullable foreign key found to resolve circular flush dependency. During flush processing, changes to instances, new instances, and deleted instances must be processed in a specific sequence to avoid foreign key constraint violations. The changes required in this transaction cannot be reordered because none of the foreign key constraints is nullable (optional).
          Hide
          Markus Fuchs added a comment -

          Thanks Craig for the review! To your comments:

          1. The graph passed to recalculateDFA should never have cycles. The assertion is testing an invariant. I abstracted the check for no cycles into a new method hasNoCycles in DepthFirstAnalysis. I re-named recalculateDFA to recalculateDepthFirstAnalysis.

          2. resolveCycles now throws a UserException, if no nullable foreign key can be found.

          3. The assertions in cycleForBackEdge are testing invariants and are therefor left in place. I moved the localization messages into a new localizer.properties file at the correct location.

          4. I took over the improved message for no-nullable-fk, which is now thown as a UserException, see above. I also improved the assertion messages.

          Question: Shall the assertion messages be localized at all?

          Show
          Markus Fuchs added a comment - Thanks Craig for the review! To your comments: 1. The graph passed to recalculateDFA should never have cycles. The assertion is testing an invariant. I abstracted the check for no cycles into a new method hasNoCycles in DepthFirstAnalysis. I re-named recalculateDFA to recalculateDepthFirstAnalysis. 2. resolveCycles now throws a UserException, if no nullable foreign key can be found. 3. The assertions in cycleForBackEdge are testing invariants and are therefor left in place. I moved the localization messages into a new localizer.properties file at the correct location. 4. I took over the improved message for no-nullable-fk, which is now thown as a UserException, see above. I also improved the assertion messages. Question: Shall the assertion messages be localized at all?
          Markus Fuchs made changes -
          Attachment openjpa-235-break-nullable-070716.patch [ 12361938 ]
          Hide
          Craig L Russell added a comment -

          Hi Markus,

          The assertion changes look good.

          It's probably best to localize the assertion messages just due to our international developer team. It's not strictly required but nice to have.

          Show
          Craig L Russell added a comment - Hi Markus, The assertion changes look good. It's probably best to localize the assertion messages just due to our international developer team. It's not strictly required but nice to have.
          Hide
          Craig L Russell added a comment -

          I tested this patch by running the mvn install command and no errors were reported.
          Markus ran the TCK and no errors were reported.

          svn commit -m "OPENJPA-235 break-nullable-patch contributed by Markus Fuchs"
          Sending openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/ConstraintUpdateManager.java
          Sending openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/ForeignKey.java
          Sending openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/kernel/localizer.properties
          Sending openjpa-lib/src/main/java/org/apache/openjpa/lib/graph/DepthFirstAnalysis.java
          Sending openjpa-lib/src/main/java/org/apache/openjpa/lib/graph/Edge.java
          Adding openjpa-lib/src/main/resources/org/apache/openjpa/lib/graph
          Adding openjpa-lib/src/main/resources/org/apache/openjpa/lib/graph/localizer.properties
          Sending openjpa-lib/src/test/java/org/apache/openjpa/lib/graph/TestDepthFirstAnalysis.java
          Sending openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/jdbc/kernel/EntityB.java
          Sending openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/jdbc/kernel/EntityC.java
          Sending openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/jdbc/kernel/EntityD.java
          Adding openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/jdbc/kernel/EntityE.java
          Sending openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/jdbc/kernel/TestNoForeignKeyViolation.java
          Transmitting file data ............
          Committed revision 557089.

          Show
          Craig L Russell added a comment - I tested this patch by running the mvn install command and no errors were reported. Markus ran the TCK and no errors were reported. svn commit -m " OPENJPA-235 break-nullable-patch contributed by Markus Fuchs" Sending openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/ConstraintUpdateManager.java Sending openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/ForeignKey.java Sending openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/kernel/localizer.properties Sending openjpa-lib/src/main/java/org/apache/openjpa/lib/graph/DepthFirstAnalysis.java Sending openjpa-lib/src/main/java/org/apache/openjpa/lib/graph/Edge.java Adding openjpa-lib/src/main/resources/org/apache/openjpa/lib/graph Adding openjpa-lib/src/main/resources/org/apache/openjpa/lib/graph/localizer.properties Sending openjpa-lib/src/test/java/org/apache/openjpa/lib/graph/TestDepthFirstAnalysis.java Sending openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/jdbc/kernel/EntityB.java Sending openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/jdbc/kernel/EntityC.java Sending openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/jdbc/kernel/EntityD.java Adding openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/jdbc/kernel/EntityE.java Sending openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/jdbc/kernel/TestNoForeignKeyViolation.java Transmitting file data ............ Committed revision 557089.
          Hide
          Reece Garrett added a comment -

          Markus,

          Your work on circular dependencies looks great, thanks for taking the time to do that! I was wondering if you are working on, or planning to working on, dependencies based on unique key constraints?

          -Reece

          Show
          Reece Garrett added a comment - Markus, Your work on circular dependencies looks great, thanks for taking the time to do that! I was wondering if you are working on, or planning to working on, dependencies based on unique key constraints? -Reece
          Hide
          Markus Fuchs added a comment -

          Reece,

          I was quite busy this week, but I will start on addressing unique constraint dependencies next week. Sorry for the late reply!

          – markus.

          Show
          Markus Fuchs added a comment - Reece, I was quite busy this week, but I will start on addressing unique constraint dependencies next week. Sorry for the late reply! – markus.
          Hide
          Reece Garrett added a comment -

          Markus,

          That's great news. No worries about late replies; I know exactly what you mean about being busy. I'm just grateful to have you working on this. The project I'm on has a growing number of uniques in the database schema and I cringe at the thought of having to write app-level business rules to deal with them. I wish all DBMS would support deferred constraints!

          -Reece

          Show
          Reece Garrett added a comment - Markus, That's great news. No worries about late replies; I know exactly what you mean about being busy. I'm just grateful to have you working on this. The project I'm on has a growing number of uniques in the database schema and I cringe at the thought of having to write app-level business rules to deal with them. I wish all DBMS would support deferred constraints! -Reece
          Craig L Russell made changes -
          Fix Version/s 0.9.8 [ 12312446 ]
          Fix Version/s 1.1.0 [ 12312344 ]
          Hide
          Reece Garrett added a comment -

          Hey Markus,

          Have you had time to make any progress on this? I know you guys have been busy.

          -Reece

          Show
          Reece Garrett added a comment - Hey Markus, Have you had time to make any progress on this? I know you guys have been busy. -Reece
          Hide
          Roger Keays added a comment -

          As discussed on openjpa-users, the ConstraintUpdateManager breaks recursive delete operations where ON DELETE CASCADE foreign key actions are used.

          http://www.nabble.com/ON-DELETE-CASCADE-causes-OptimisticLockingException-td14308483.html

          The old OperationOrderUpdateManager doesn't have this problem.

          Show
          Roger Keays added a comment - As discussed on openjpa-users, the ConstraintUpdateManager breaks recursive delete operations where ON DELETE CASCADE foreign key actions are used. http://www.nabble.com/ON-DELETE-CASCADE-causes-OptimisticLockingException-td14308483.html The old OperationOrderUpdateManager doesn't have this problem.
          Patrick Linskey made changes -
          Fix Version/s 1.1.0 [ 12312344 ]
          Fix Version/s 1.2.0 [ 12313102 ]
          Hide
          Fay Wang added a comment -

          I did some investigation about the OptimisticLock exception where ConstraintUpdateManager is used when entity A is deleted:

          @Entity A

          { @OneToMany(cascade=CascadeType.ALL, mappedBy="a") List b; }

          @Entity B

          { @ForeignKey(deleteAction=ForeignKeyAction.CASCADE) @ManyToOne(cascade=CascadeType.MERGE) A a; }

          It is found that if the foreign key delete action is set to "on delete restrict", the deletion of A is fine without OptimisticLock exception. It turns out that
          ConstraintUpdateManager only re-order the deletions of entities for foreign key on delete restrict such that the child rows (entity B) are deleted first before the parent rows (entity A) are deleted. This re-ordering should also be applied to foreign key on delete cascade.
          With foreign key on delete cascade, if the parent row is deleted first, the child rows will be deleted automatically by the database, resulting in
          OptimiticLock exception (b/c the child rows (entity B) are no longer there when openjpa tries to delete them.)

          The attached patch adds support for foreign key on delete cascade so that the ConstraintUpdateManager will re-order the deletions of entities to prevent OptimiticLock exception.

          Show
          Fay Wang added a comment - I did some investigation about the OptimisticLock exception where ConstraintUpdateManager is used when entity A is deleted: @Entity A { @OneToMany(cascade=CascadeType.ALL, mappedBy="a") List b; } @Entity B { @ForeignKey(deleteAction=ForeignKeyAction.CASCADE) @ManyToOne(cascade=CascadeType.MERGE) A a; } It is found that if the foreign key delete action is set to "on delete restrict", the deletion of A is fine without OptimisticLock exception. It turns out that ConstraintUpdateManager only re-order the deletions of entities for foreign key on delete restrict such that the child rows (entity B) are deleted first before the parent rows (entity A) are deleted. This re-ordering should also be applied to foreign key on delete cascade. With foreign key on delete cascade, if the parent row is deleted first, the child rows will be deleted automatically by the database, resulting in OptimiticLock exception (b/c the child rows (entity B) are no longer there when openjpa tries to delete them.) The attached patch adds support for foreign key on delete cascade so that the ConstraintUpdateManager will re-order the deletions of entities to prevent OptimiticLock exception.
          Fay Wang made changes -
          Attachment openjpa.patch [ 12383396 ]
          Fay Wang made changes -
          Attachment openjpa.patch [ 12383397 ]
          Hide
          Fay Wang added a comment -

          This patch fixed a typo in the comments and also includes a test case.

          Show
          Fay Wang added a comment - This patch fixed a typo in the comments and also includes a test case.
          Fay Wang made changes -
          Attachment openjpa-235-delete-cascade.patch [ 12383416 ]
          Hide
          Bogdan Stroe added a comment -

          Actually it doesn't matter if the foreign key is nullable or not. I have the same problem even on a nullable foreign key: it gets an id for the second entity from the sequence and then it tries to insert that id as a foreign key before inserting in the other (parent) table. So you get a "parent key not found" error.

          Show
          Bogdan Stroe added a comment - Actually it doesn't matter if the foreign key is nullable or not. I have the same problem even on a nullable foreign key: it gets an id for the second entity from the sequence and then it tries to insert that id as a foreign key before inserting in the other (parent) table. So you get a "parent key not found" error.
          Hide
          Fay Wang added a comment -

          Bogdan, the problem you described appears to be an insert problem. Do you have a test case it?

          Show
          Fay Wang added a comment - Bogdan, the problem you described appears to be an insert problem. Do you have a test case it?
          Hide
          Roger Keays added a comment -

          Hi Fay, unfortunately your patch doesn't seem to work for me. However, as you discovered it seems that using RESTRICT in the annotations does work around the problem. This appears to work even if the actual database constraints are set to CASCADE.

          Show
          Roger Keays added a comment - Hi Fay, unfortunately your patch doesn't seem to work for me. However, as you discovered it seems that using RESTRICT in the annotations does work around the problem. This appears to work even if the actual database constraints are set to CASCADE.
          Hide
          Fay Wang added a comment -

          Hi Roger, my patch includes a test case to test deletion for foreign key on delete cascade. This test case will not work without the patch. Since yours still does not work, can you provide me your test case or let me know the difference between your test case and mine for further investigation? Thanks!

          Show
          Fay Wang added a comment - Hi Roger, my patch includes a test case to test deletion for foreign key on delete cascade. This test case will not work without the patch. Since yours still does not work, can you provide me your test case or let me know the difference between your test case and mine for further investigation? Thanks!
          Hide
          Roger Keays added a comment -

          Hi Fay, Your patch does actually work for me. I just needed to clean and rebuild. Thanks for that, I was tearing my hair out. Will we see the patch applied for 1.1.1?

          Show
          Roger Keays added a comment - Hi Fay, Your patch does actually work for me. I just needed to clean and rebuild. Thanks for that, I was tearing my hair out. Will we see the patch applied for 1.1.1?
          Hide
          Bogdan Stroe added a comment -

          Fay, I downloaded one of the existing test cases and I will prepare a similar one for my problem.

          Show
          Bogdan Stroe added a comment - Fay, I downloaded one of the existing test cases and I will prepare a similar one for my problem.
          Hide
          Michael Dick added a comment -

          Is there more work to be done here, or can we consider the issue resolved?

          Show
          Michael Dick added a comment - Is there more work to be done here, or can we consider the issue resolved?
          Hide
          Michael Dick added a comment -

          I believe this issue is resolved. If there are other cases which need addressing please open a separate JIRA or a subtask.

          Show
          Michael Dick added a comment - I believe this issue is resolved. If there are other cases which need addressing please open a separate JIRA or a subtask.
          Michael Dick made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Fix Version/s 1.3.0 [ 12313326 ]
          Hide
          David Ezzio added a comment -

          Merged fix to 1.1.x branch at 802195 from trunk.

          Show
          David Ezzio added a comment - Merged fix to 1.1.x branch at 802195 from trunk.
          David Ezzio made changes -
          Fix Version/s 1.1.1 [ 12313177 ]
          Donald Woods made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Patrick Linskey
              Reporter:
              Reece Garrett
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development