NH-3931 - Invalid order of child inserts when using TPH inheritance #577

fredericDelaporte · 2017-03-22T02:09:40Z

Test cases and fix for NH-3931.

Ported from HHH-9864 and HHH-11216.

Some additional changes were required on NHibernate side, see comments on file ActionQueue.

fredericDelaporte · 2017-03-22T02:11:56Z

src/NHibernate/Engine/ActionQueue.cs

@@ -107,17 +109,17 @@ public void AddAction(BulkOperationCleanupAction cleanupAction)
 		{
 			RegisterCleanupActions(cleanupAction);
 		}
-


While working, I have ended up CTRL+K CTRL+D due to wrecked indentation in Java ported code for the fix, without noticing the amount of bad spaces already littering this file. Actual changes start at ligne 507.

fredericDelaporte · 2017-03-22T02:21:34Z

src/NHibernate/Engine/ActionQueue.cs

+				{
+					log.WarnFormat("Entity {0} persister does not provide meta-data, its batch ordering may be wrong and cause a failure.",
+						batchIdentifier.EntityName);
+					return;


See code comment above.
Both on Hibernate side and NHibernate side, the ClassMetadata has following reference code comment:

Get the user-visible metadata for the class (optional operation)

Does that mean we have to support null when calling this property? Not "supporting" it was causing a test, Legacy.MasterDetailTest, to fail.

The "support" I have implemented just give up ordering. Obviously a project using some custom provider without meta-data will very likely run into troubles with this change. Must we support this case, custom provider without meta-data?

Can you trace what has happened to Legacy.MasterDetailTest in Hibernate?

Still there. There are some changes but getClassMetadata still yield null.

Hibernate does not have insert ordering and batching enabled by default. This test on their side may run unbatched. It would explain why they do not see this failure, neither maybe the one-to-one failure we have below.

I have left them some comments.

They have "solved" it the same way: no ordering in case the persister does not provide class meta data. But I have noticed that on their side, their custom provider does provide an entity meta model while ours does not. Maybe syncing our custom provider with their and fallback-ing to the entity meta model could work.

fredericDelaporte · 2017-03-22T02:25:05Z

src/NHibernate/Engine/ActionQueue.cs

+						var entityType = (EntityType)type;
+						// NHibernate has non constrained one-to-one among its state while it seems Hibernate does not.
+						// We have to filter them out, since they are indeed children, not parents.
+						if (!(entityType.IsOneToOne && entityType.IsNullable))


See code comment, either Hibernate has still a bug with those changes, or Hibernate does not fetch the same data in action.State.

Without that, I get both one-to-one sides referencing each other as parent. This causes the sorting algorithm to swap them, even if the actual parent was the first. 14 tests were failing before I changed that.

hazzik · 2017-03-22T02:44:49Z

src/NHibernate/Engine/ActionQueue.cs

+					var entityName = batchIdentifier.EntityName;
+
+					//Make sure that child entries are not before parents
+					for (var j = i - 1; j >= 0; j--)


While everything is great with your work, I think that this 2 (3) for loops are complete crap.

Yes, but well, sorting algorithms never gets done without some looping... And I am not very knowledgeable about efficient sorting algorithms. At least the lookup are done as O(1) with set. (By the way, Hibernate was doing a lookup in a list, I have change that for a dictionary while porting.)

Throwing in some usual sorting algorithm will probably not be easy, since there is no actual comparison between elements. Many of them could be "neutral" with most others, while requiring an order with some. So those "neutral" elements may not be contiguous in a correctly sorted result, which may cause usual sorting algorithms to fail.

Still thinking about it, this sort is O(N²). N is the number of distinct entity classes involved in the flush. If this gets merged, we may document that session having a lot of distinct entity classes to flush at once would trigger a costly queries sort if ordering is enabled.

In principle a comparison based sort is Ω(N log N) (cannot be better than that), which already does not scale well. So with our added constraint of not being able of comparing all elements, I do not think we can do better than this O(N²).

About comparing all elements, we may have two unrelated set of entity classes, 1: A < B < C and 2: D < E < F, requiring ordering among themselves but not caring about the other set. When comparing one entity from a set (B by example) to an entity class of the other set (let's take F), they should not be considered equals.
Otherwise a comparison based sorting algorithm may encounter B == F == A while A must be < B, which would wreck the algorithm.

So for using a comparison based algorithm, we would have to detect each set of comparable entity classes in the batch, assign them a global ranking arbitrarily, and use it when comparing entity classes across sets: could be worth it on big sorts, but normally a session is not supposed to have a big list of entity classes to handle in the same flush. (Note that ado.batch_size does not limit the number of classes to order, as of my understanding. It only causes the resulting set of queries for a class to get split after ordering.)

I was almost able to achieve the sort by doing OrderBy+ThenBy, but it didn't completely work.

By merging ChildrenNames and ParentNames into single collection I was able to implement just a bubble sort algo.

Actually, we have even worse performance. It's close to N^6 (but Hibernate has N^2)

List<> are that bad about removals and insertions? I should have checked that.

Well, O(n) ops, this causes the global sort to be O(n^3) on NHibernate side. How do you come to O(n^6)? ( O(2n^3) is indeed equivalent to O(n^3), such factor is usually just dropped in O notation.)

Just double check everything I'm saying - it's 2:45 AM here:)

And well, there is a LinkedList<>, I just have to change for it and take care about that next time I have to deal with inserts and removals.

fredericDelaporte · 2017-03-22T03:13:33Z

Fixed NH-3931 test cases restriction: it can not run with ODBC. Still some other tests are failing: an optimistic concurrency test with ODBC; and a bunch of tests under Firebird seems to have ordering issues. I will look at that later.

fredericDelaporte · 2017-03-22T03:34:43Z

It was not very logic to ignore one-to-one detected as children, not parents. Maybe this was the Firebird failure cause. Now I add them in the children set.

hazzik · 2017-03-22T03:43:42Z

src/NHibernate/Engine/ActionQueue.cs

+			// _orderedBatches and _entityNameBatchIdentifier replace _latestBatch from Hibernate implementation.
+			private readonly List<BatchIdentifier> _orderedBatches = new List<BatchIdentifier>();
+			private readonly Dictionary<string, BatchIdentifier> _entityNameBatchIdentifier = new Dictionary<string, BatchIdentifier>();
+			private readonly Dictionary<object, BatchIdentifier> _entityBatchIdentifier;


Content of _entityBatchIdentifier is not used, so I think we need to remove it.

hazzik · 2017-03-22T03:44:20Z

src/NHibernate/Engine/ActionQueue.cs

+
+			// _orderedBatches and _entityNameBatchIdentifier replace _latestBatch from Hibernate implementation.
+			private readonly List<BatchIdentifier> _orderedBatches = new List<BatchIdentifier>();
+			private readonly Dictionary<string, BatchIdentifier> _entityNameBatchIdentifier = new Dictionary<string, BatchIdentifier>();


_orderedBatches and _entityNameBatchIdentifier are used only within Sort method, please demote them to local variables.

hazzik · 2017-03-22T03:55:50Z

src/NHibernate/Engine/ActionQueue.cs

+						_entityNameBatchIdentifier[action.EntityName] = batchIdentifier;
+						_orderedBatches.Add(batchIdentifier);
+					}
+					AddParentChildEntityNames(action, batchIdentifier);


I think this should be inside if block above as this need to be preformed only once.

Ah, this is to "optimize" dynamic inserts.

hazzik · 2017-03-22T04:19:30Z

src/NHibernate.Test/NHSpecificTest/NH3931/Entities.cs

+using System;
+using System.Collections.Generic;
+
+namespace NHibernate.Test.NHSpecificTest.NH3931


Could you please move tests to NHibernate.Test.Insertordering where the old one lives?

Could you please move tests to NHibernate.Test.Insertordering where the old one lives?

Done, completed with HHH-11585 additional tests. They have added an additional bug on their side by badly simplifying my fix for one-to-one case. So I have not ported that. I have removed the useless dictionary though.

I have not worked on the flawed sorting algorithm today, I will do maybe tomorrow.

…with mixed subclass entities

fredericDelaporte · 2017-03-22T10:59:30Z

On Hibernate side, they have committed an additional patch for handling one-to-one (HHH-11585) and the CustomProvider.
It is almost same as what I have done. I will adjust this PR to be closer (remove an useless condition) to Hibernate resulting implementation.

hazzik · 2017-03-22T13:52:26Z

src/NHibernate/Engine/ActionQueue.cs

+						var prevBatchIdentifier = _orderedBatches[j];
+						if (prevBatchIdentifier.ParentEntityNames.Contains(entityName))
+						{
+							_orderedBatches.RemoveAt(i);


IF we'll have 2 prevBatchIdentifiers matching the criteria, we'll throw out to the void the second element which'll get to the i place and will add the second batchIdentifier to the list?

Well, it looks like, well spotted... All this change seems a hell buggy on Hibernate side...

Just loud thinking (I did not check though and don't have time to check now): This is, probably, what happens on Oracle and Firebird. Most likely it got exposed because these tests use native generator which is identity on MSSQL (identity prevents inserts batching and so sorting), but on Oracle and Firebird native generator is sequence, which allows batching and sorting.

these = failing*

Hum, using a LinkedList could bring us back to O(n²) and avoid this kind of issues, since we will be able to work with nodes instead of indexes. It should be way more readable, and so easier to write correctly.

I will give it a try.

Yes, once I had understood your point, I just told me "hopefully that is the cause of those Teamcity build failures". We should copy those tests to insertorder and use something like guid.comb.

(And maybe is it time for you to go have a good night. I have stayed till 4am "yesterday" (technically this morning indeed) but I did not have to go to work today. You are in the -11 time zone? That is what I compute when looking at hours. I am in the +1.)

No, it's +12 here in NZ, I live in the future:)

hazzik · 2017-03-22T20:04:05Z

I've briefly checked Can_discriminate_subclass_on_list_with_lazy_loading_when_used_get test with the "hilo" generator which permits batching. As I thought, Sort method just corrupts the ordering: duplicating some batches and dropping others.

NHibernate.Test.NHSpecificTest.NH1391.Animal
NHibernate.Test.NHSpecificTest.NH1391.Dog
NHibernate.Test.NHSpecificTest.NH1391.Cat
NHibernate.Test.NHSpecificTest.NH1391.PersonWithAnimals
NHibernate.Test.NHSpecificTest.NH1391.SivasKangal
NHibernate.Test.NHSpecificTest.NH1391.PersonWithCats
NHibernate.Test.NHSpecificTest.NH1391.PersonWithDogs
NHibernate.Test.NHSpecificTest.NH1391.PersonWithSivasKangals

"Sorts" to:

NHibernate.Test.NHSpecificTest.NH1391.Animal
NHibernate.Test.NHSpecificTest.NH1391.PersonWithCats
NHibernate.Test.NHSpecificTest.NH1391.Animal
NHibernate.Test.NHSpecificTest.NH1391.PersonWithSivasKangals
NHibernate.Test.NHSpecificTest.NH1391.Animal
NHibernate.Test.NHSpecificTest.NH1391.Animal
NHibernate.Test.NHSpecificTest.NH1391.SivasKangal
NHibernate.Test.NHSpecificTest.NH1391.Animal

fredericDelaporte · 2017-03-24T00:39:13Z

Animal test case ported into Insertordering tests. It confirms it fails with NH-3931 current state, but works with previous code of ActionQueue (locally tested).

I have also added a circular references test: it works with previous code, but not with current. Current code is completely unaware of circular references.

I wonder if we should actually fix ported Hibernate code, which solve some unsupported cases by causing a lot of older supported cases to fail, or come back to older code and fix older code instead.

hazzik · 2017-03-24T01:11:04Z

I prefer this:

come back to older code and fix older code instead

To review when it's "green"

hazzik · 2017-03-26T23:12:34Z

Closing in favor of #582

fredericDelaporte commented Mar 22, 2017

View reviewed changes

hazzik reviewed Mar 22, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-3931 branch from 21bdb25 to d5e6510 Compare March 22, 2017 03:09

fredericDelaporte force-pushed the NH-3931 branch from d5e6510 to f4424ab Compare March 22, 2017 03:32

hazzik previously requested changes Mar 22, 2017

View reviewed changes

fredericDelaporte referenced this pull request in hibernate/hibernate-orm Mar 22, 2017

HHH-9864 - foreign key violation with order_inserts=true and batches …

0c8261b

…with mixed subclass entities

hazzik reviewed Mar 22, 2017

View reviewed changes

fredericDelaporte added 2 commits March 22, 2017 18:47

NH-3931 - test cases ported from HHH-9864, HHH-11216 and HHH-11585

86c76e6

NH-3931 - port of Hibernate fix HHH-9864, HHH-11216 and HHH-11585.

c609112

fredericDelaporte force-pushed the NH-3931 branch from f4424ab to c609112 Compare March 22, 2017 19:17

hazzik added the needs work label Mar 23, 2017

NH-3931 - additional test cases

b62bcf6

fredericDelaporte force-pushed the NH-3931 branch from f05a9fb to b62bcf6 Compare March 24, 2017 00:34

fredericDelaporte mentioned this pull request Mar 26, 2017

NH-3931 - Invalid order of child inserts #582

Merged

hazzik closed this Mar 26, 2017

fredericDelaporte deleted the NH-3931 branch March 28, 2017 09:44

fredericDelaporte added r: Replaced t: Bug and removed needs work labels Sep 6, 2017

NH-3931 - Invalid order of child inserts when using TPH inheritance #577

NH-3931 - Invalid order of child inserts when using TPH inheritance #577

Uh oh!

Conversation

fredericDelaporte commented Mar 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Mar 22, 2017

Uh oh!

fredericDelaporte commented Mar 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

fredericDelaporte Mar 22, 2017 •

edited

Loading

fredericDelaporte Mar 22, 2017 •

edited

Loading

fredericDelaporte Mar 22, 2017 •

edited

Loading

fredericDelaporte Mar 22, 2017 •

edited

Loading

fredericDelaporte Mar 22, 2017 •

edited

Loading

fredericDelaporte commented Mar 22, 2017 •

edited

Loading

fredericDelaporte Mar 22, 2017 •

edited

Loading

hazzik commented Mar 22, 2017 •

edited

Loading

fredericDelaporte commented Mar 24, 2017 •

edited

Loading