[
https://issues.apache.org/jira/browse/OPENJPA-235?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12512735
]
Craig Russell commented on OPENJPA-235:
---------------------------------------
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).
> SQL reordering to avoid non-nullable foreign key constraint violations
> ----------------------------------------------------------------------
>
> Key: OPENJPA-235
> URL: https://issues.apache.org/jira/browse/OPENJPA-235
> Project: OpenJPA
> Issue Type: Improvement
> Components: kernel
> Reporter: Reece Garrett
> Assignee: Patrick Linskey
> Fix For: 0.9.8
>
> Attachments: merge-detached.patch, merge-testcases.patch,
> openjpa-235-break-nullable.patch, openjpa-235-test.jar,
> openjpa-235-test1.jar, openjpa-235-test2.zip, sqlreorder.patch,
> sqlReorder2.patch, sqlReorderTests.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
