Join-fetching with property-ref results in select n+1 #1492

fredericDelaporte · 2017-12-16T11:31:27Z

Fixes NH-2534 - Join-fetching a many-to-one with property-ref results in select n+1 problem #1226 (NH-2534)

This is a rebase of #1460, with async regen, squashed, as it was asked here.

hazzik · 2017-12-16T11:43:27Z

src/NHibernate/Loader/Loader.cs

@@ -972,6 +972,29 @@ private void CheckVersion(int i, IEntityPersister persister, object id, object e
 					entry.LockMode = lockMode;
 				}
 			}
+
+			// #1226: If it is already loaded and can be loaded from an association with a property ref, make


Can this please be extracted into an own method?

hazzik · 2017-12-16T11:44:59Z

@fredericDelaporte why not do it in the original PR? It seems to have "changes from contributors" enabled.

fredericDelaporte · 2017-12-16T11:52:57Z

It seems to have "changes from contributors" enabled.

Yes but I have seen on some other PR that some people have difficulties handling forced pushes. So unless the contributor has already shown by its action on his PR that it is not a trouble for him, I now avoid doing forced pushes on their PR.

gliljas · 2017-12-17T07:54:51Z

@fredericDelaporte Yes, force pushing on someone else's PR should only be done after communicating with the owner about it.

* Fixes nhibernate#1226 (NH-2534)

hazzik · 2017-12-20T11:21:30Z

@fredericDelaporte, @kfedorchenko can the CacheByUniqueKey logic be unifed, eg. called this way:

//If the object is already loaded, return the loaded one
obj = session.GetEntityUsingInterceptor(key);
if (obj != null)
{
	//its already loaded so dont need to hydrate it
	InstanceAlreadyLoaded(rs, i, persisters[i], key, obj, lockModes[i], session);
}
else
{
	obj = InstanceNotYetLoaded(rs, i, persisters[i], key, lockModes[i], descriptors[i].RowIdAlias, optionalObjectKey, optionalObject, hydratedObjects, session);
}
CacheByUniqueKey(i, persisters[i], obj, session);

And remove this logic from LoadFromResultSet?

(I don't know if this is possible or not, it just a suggestion)

fredericDelaporte · 2017-12-20T12:36:18Z

can the CacheByUniqueKey logic be unifed

No, there is a trap here: in the InstanceNotYetLoaded case, obj is not actually hydrated but just put in a "to be hydrated" list, with its values array already put in persistence context. So getting its unique key value from its properties as do CacheByUniqueKey will fail. We may try to get values back from the persistence context in such case, but this would cause a more complex code and also more redundant unique key caching when the entity appears in many rows.

hazzik · 2017-12-20T12:52:40Z

I've discovered this "trap".

this code works:

obj = session.GetEntityUsingInterceptor(key);
var persister = persisters[i];
object[] values;
if (obj != null)
{
	//its already loaded so dont need to hydrate it
	InstanceAlreadyLoaded(rs, i, persister, key, obj, lockModes[i], session);
	values = persister.GetPropertyValues(obj);
	CacheByUniqueKey(i, persister, obj, session, values);
}
else
{
	obj = InstanceNotYetLoaded(rs, i, persister, key, lockModes[i], descriptors[i].RowIdAlias, optionalObjectKey, optionalObject, hydratedObjects, session);
	var entry = session.PersistenceContext.GetEntry(obj);
	values = entry.LoadedState;
}
CacheByUniqueKey(i, persister, obj, session, values);

hazzik

Minor code issues, otherwise looks good.

hazzik · 2017-12-20T13:04:11Z

src/NHibernate/Loader/Loader.cs

+		{
+			// #1226: If it is already loaded and can be loaded from an association with a property ref, make
+			// sure it is also cached by its unique key.
+			var ukName = OwnerAssociationTypes?[i]?.RHSUniqueKeyPropertyName;


can we unfold ? operator here? It looks unreadable.

Just null propagation all the way. Original code was:

IAssociationType[] ownerAssociationTypes = OwnerAssociationTypes; if (ownerAssociationTypes != null && ownerAssociationTypes[i] != null) { string ukName = ownerAssociationTypes[i].RHSUniqueKeyPropertyName;

Would you rather have:

var ownerAssociationTypes = OwnerAssociationTypes; if (ownerAssociationTypes == null) return; var ukName = ownerAssociationTypes[i]?.RHSUniqueKeyPropertyName;

hazzik · 2017-12-20T13:04:46Z

src/NHibernate.Test/NHSpecificTest/GH1226/Fixture.cs

+				using (var tx = session.BeginTransaction())
+				{
+					session.Delete("from Account");
+					session.Delete("from Bank");


CreateQuery("delete ...").ExecuteUpdate()?

hazzik · 2017-12-20T13:05:40Z

src/NHibernate.Test/NHSpecificTest/GH1226/Fixture.cs

+
+			using (var session = OpenSession())
+			{
+				using (var tx = session.BeginTransaction())


please align nested usings:

using(...) using(...) { }

hazzik · 2017-12-20T13:05:56Z

src/NHibernate.Test/Async/NHSpecificTest/GH1226/Fixture.cs

+
+			using (var session = OpenSession())
+			{
+				using (var tx = session.BeginTransaction())


hazzik · 2017-12-20T13:08:44Z

src/NHibernate/Loader/Loader.cs

+			if (ukValue == null)
+				return;
+			var type = persister.PropertyTypes[index];
+			var euk = new EntityUniqueKey(persister.EntityName, ukName, ukValue, type, session.Factory);


LoadFromRow's version caches by rootPersister.EntityName. Please add a test where the referenced entity is a hierarchical object.

Here persister is rootPersister. Non-root persister is a thing existing only inside LoadFromResultSet.

fredericDelaporte · 2017-12-20T14:29:53Z

this code works:

But it causes, in the already loaded case, to get all the entity properties from its tuplizer even if there is no unique key, which is quite heavier than getting only one property when there is an unique key. I would rather pass null as values for this case, and fallback on the single property.
Well I consider instead passing an alreadyLoaded boolean and handle that inside the method.

hazzik · 2017-12-20T23:59:35Z

src/NHibernate.Test/NHSpecificTest/GH1226/Fixture.cs

+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				// Bug only occurs if the Banks are already in the session cache.


Since the old code has changed, could this case please be checked as well?

You mean, checking eager fetching works for Bank not already in session too? It is likely this is already tested somewhere else, but why not.

Fix key type mismatch in EntityUniqueKey EntityUniqueKey was assuming the value to be always semi-resolved (to be the identifier instead of the entity for an entity type). It was always computing the value hashcode with the semi-resolved type, which is the identifier type for an entity. But this has changed with nhibernate#1492, which caches unique keys also for already fully loaded entities. The value hashcode must then be computed with the entity type, which accounts for proxies. Otherwise the proxy loading may get triggered while the code is already loading the proxy, which results in a stack overflow. Add test case adapted from Michael Estermann repro Fix nhibernate#1645 Co-authored-by: Michael Estermann <michael.estermann@bbv.ch>

Fix key type mismatch in EntityUniqueKey EntityUniqueKey was assuming the value to be always semi-resolved (to be the identifier instead of the entity for an entity type). It was always computing the value hashcode with the semi-resolved type, which is the identifier type for an entity. But this has changed with #1492, which caches unique keys also for already fully loaded entities. The value hashcode must then be computed with the entity type, which accounts for proxies. Otherwise the proxy loading may get triggered while the code is already loading the proxy, which results in a stack overflow. Add test case adapted from Michael Estermann repro Fix #1645 Co-authored-by: Michael Estermann <michael.estermann@bbv.ch>

fredericDelaporte added the t: Fix label Dec 16, 2017

fredericDelaporte added this to the 5.1 milestone Dec 16, 2017

fredericDelaporte mentioned this pull request Dec 16, 2017

NH-2534 - Join-fetching a many-to-one with property-ref results in se… #1460

Closed

hazzik reviewed Dec 16, 2017

View reviewed changes

Join-fetching with property-ref results in n+1 select

00b36c0

* Fixes nhibernate#1226 (NH-2534)

fredericDelaporte force-pushed the 1226 branch from b443a86 to 00b36c0 Compare December 18, 2017 11:24

hazzik reviewed Dec 20, 2017

View reviewed changes

fredericDelaporte and others added 3 commits December 20, 2017 15:31

Mutualize caching by unique key.

912d210

Tests adjustment and stale data case.

c628196

Merge branch 'master' into 1226

2d610b1

hazzik reviewed Dec 20, 2017

View reviewed changes

Add nominal case test.

7b74b9d

hazzik approved these changes Dec 22, 2017

View reviewed changes

fredericDelaporte merged commit ba8fb89 into nhibernate:master Dec 22, 2017

fredericDelaporte deleted the 1226 branch December 22, 2017 11:13

fredericDelaporte added the r: Fixed label Dec 22, 2017

fredericDelaporte mentioned this pull request Apr 9, 2018

One-to-one with property-ref triggers StackOverflow Exception #1645

Closed

fredericDelaporte mentioned this pull request Apr 9, 2018

Fix key type mismatch in EntityUniqueKey #1647

Merged

fredericDelaporte mentioned this pull request Dec 2, 2018

NH-2285 - Support for LockMode in Linq provider #530

Merged

Join-fetching with property-ref results in select n+1 #1492

Join-fetching with property-ref results in select n+1 #1492

Uh oh!

Conversation

fredericDelaporte commented Dec 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented Dec 16, 2017

Uh oh!

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

Uh oh!

Uh oh!

gliljas commented Dec 17, 2017

Uh oh!

hazzik commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Dec 20, 2017

Uh oh!

hazzik commented Dec 20, 2017

Uh oh!

hazzik left a comment

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!

hazzik Dec 20, 2017 • edited by fredericDelaporte 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!

fredericDelaporte commented Dec 20, 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!

Uh oh!

fredericDelaporte commented Dec 16, 2017 •

edited

Loading

hazzik commented Dec 20, 2017 •

edited

Loading

hazzik Dec 20, 2017 •

edited by fredericDelaporte

Loading