Fix for update with outer join in PostgreSQL #1568

Doan-Van-Tuan · 2018-02-01T07:35:22Z

Add alias in "for update" clause to entity loading query containing outer join if database (e.g Postgres) does not support locking with outer join.

Fixes #1565.

Doan-Van-Tuan · 2018-02-01T10:11:28Z

Hi, please help me with the test failure on TeamCity. I'm not sure how to make it only runs with Postgres.

bahusoid · 2018-02-01T10:25:05Z

I think your Configure override needs to be removed. It seems it forces Postgres dialect on all configurations.

hazzik · 2018-02-01T10:32:20Z

@Doan-Van-Tuan, @bahusoid is right.

fredericDelaporte · 2018-02-01T10:42:11Z

And also, please reformat the .cs test files added in this PR. There is a mix of tabs and spaces, NHibernate uses tabs.

Doan-Van-Tuan · 2018-02-01T15:23:14Z

Thank you. I'm not familiar with NHibernate test configurations and conventions.

fredericDelaporte · 2018-02-01T21:21:32Z

src/NHibernate.Test/NHSpecificTest/GH1565/PostgresOuterJoinLockTest.cs

+
+		protected override bool AppliesTo(Dialect.Dialect dialect)
+		{
+			return dialect is PostgreSQLDialect;


This limitation should be removed. The lock should work for every database, better test them all. So remove the AppliesTo override.

AppliesTo() was removed.

fredericDelaporte · 2018-02-01T22:13:00Z

src/NHibernate/SqlCommand/SqlSelectBuilder.cs

@@ -181,9 +187,10 @@ public SqlSelectBuilder SetHavingClause(SqlString havingSqlString)
 			return this;
 		}

-		public SqlSelectBuilder SetLockMode(LockMode lockMode)
+		public SqlSelectBuilder SetLockMode(LockMode lockMode, string alias)


This is a binary breaking change. Changing a public method must be done in two steps: obsolete the method, add an overload. The alias parameter could be a bit more explicit about what it is, such as fromTableAlias. Since the for update can accept a list of tables, it could even be something like nonOuterAliases.

fredericDelaporte · 2018-02-01T22:17:24Z

src/NHibernate/SqlCommand/SqlSelectBuilder.cs

@@ -40,6 +42,7 @@ public SqlSelectBuilder SetComment(string comment)
 		public SqlSelectBuilder SetFromClause(string fromClause)
 		{
 			this.fromClause = fromClause;
+			hasOuterJoin = hasOuterJoin || fromClause.ToLowerInvariant().Contains("left outer join");


Better use this code for a case insensitive Contains. It has been merge just now, so update your branch for having this StringHelper.ContainsCaseInsensitive method.

The test should be moved into GetForUpdateString, and done only if the dialect does not support for update on outer joins. It would avoid doing such a string scan for all other cases.

Changed to StringHelper.ContainsCaseInsensitive; and moved the check for outer join into GetForUpdateString().

fredericDelaporte · 2018-02-01T22:22:13Z

src/NHibernate/SqlCommand/SqlSelectBuilder.cs

@@ -110,6 +113,9 @@ public SqlSelectBuilder SetOuterJoins(SqlString outerJoinsAfterFrom, SqlString o
 			}

 			this.outerJoinsAfterWhere = tmpOuterJoinsAfterWhere;
+
+			hasOuterJoin = hasOuterJoin || 
+			               outerJoinsAfterFrom != null && outerJoinsAfterFrom.ToString().ToLowerInvariant().Contains("outer join");


The test should be moved into GetForUpdateString, and done only if the dialect does not support for update on outer joins. It would avoid doing such a string scan for all other cases.

By the way, is it really required here to test the outerJoinsAfterFrom contains an outer join? Shouldn't it be enough to test the string is not null or just whitespaces?

fredericDelaporte · 2018-02-01T22:42:55Z

src/NHibernate/SqlCommand/SqlSelectBuilder.cs

+			if (!Dialect.SupportsOuterJoinForUpdate && hasOuterJoin)
+			{
+				if (Equals(lockMode, LockMode.Upgrade))
+					return Dialect.GetForUpdateString(aliasToLock);


We still have a trouble here: databases not supporting for update on outer join and not supporting for update aliases. For such database the code here will keep a for update clause, and it will still fail.

So either a new property needs to be added for knowing if the dialect supports lock aliases (currently, if it does not, it yields the for update without aliases), and used here, or the for update should just yield here an empty string, removing the need for transmitting the alias by the way.

Is there any DBMS that matches? I'd like to leave this logic for a later date when we need it.

Maybe not, but see my update on #1565, it is definitely incorrect to systematically supply a table alias here. Some DB do take indeed a column list here, not a table alias. The correct code handling both cases is in ForUpdateFragment.

…ining outer join if database (e.g Postgres) does not support locking with outer join

- Restore and mark SetLockMode(LockMode) obsolete - Only check for outer join if the dialect is not support locking entire outer join - Rename test class to reflect that it not limited to PostgreSQL

bahusoid · 2018-02-03T06:10:57Z

src/NHibernate/SqlCommand/SqlSelectBuilder.cs

+			{
+				return
+					StringHelper.ContainsCaseInsensitive(fromClause, "outer join") ||
+					!string.IsNullOrWhiteSpace(outerJoinsAfterFrom?.ToString());


It's better to swap those conditions and if empty/whitespace check is really required use SqlString.IsEmptyOrWhitespace

nhibernate-core/src/NHibernate/SqlCommand/SqlString.cs

Line 658 in f8b1bc4

public bool IsEmptyOrWhitespace()

outerJoinsAfterFrom?.IsEmptyOrWhitespace() == false

@Doan-Van-Tuan could you please incorporate this suggestion?

I'll resume work on this fix in next few days. It was our Lunar New Year.

…k-issue

hazzik · 2018-03-12T23:36:27Z

@fredericDelaporte could you please review this to include in 5.1. It seems only minor request is missing. I'm going to adjust it myself.

… bahusoid.

fredericDelaporte

Approving up to Remove undue whitespace addition and do condition swap recommended by bahusoid commit.

The other commits I have added should be approved by another reviewer I think.

fredericDelaporte · 2018-03-13T12:04:37Z

src/NHibernate/Dialect/Dialect.cs

+
+		/// <summary>Is <c>FOR UPDATE OF</c> syntax expecting columns?</summary>
+		/// <value><see langword="true"/> if the database expects a column list with <c>FOR UPDATE OF</c> syntax,
+		/// <see langword="false"/> if it expects table alias instead or do not support <c>FOR UPDATE OF</c> syntax.</value>
 		public virtual bool ForUpdateOfColumns


Its previous comment was wrong. It is actually used to determine whether columns aliases (true value) or tables aliases (false value) have to be supplied to GetForUpdateString(string aliases). Then GetForUpdateString(string aliases) implementation will ignore supplied aliases if the dialect is not supporting for update of at all.

It still does that without relying on the new ForUpdateOf I have added.

fredericDelaporte · 2018-03-13T12:11:19Z

src/NHibernate/SqlCommand/SqlSelectBuilder.cs

+			{
+				var isUpgrade = Equals(lockMode, LockMode.Upgrade);
+				var isUpgradeNoWait = !isUpgrade && (
+					Equals(lockMode, LockMode.UpgradeNoWait) || Equals(lockMode, LockMode.Force));


Adding forgotten Force case by the way. Previously this case would have fallback on Dialect.GetForUpdateString(LockMode) which would then have emit an unsupported for update clause.

fredericDelaporte · 2018-03-13T12:14:01Z

src/NHibernate/SqlCommand/SqlSelectBuilder.cs

+				var isUpgradeNoWait = !isUpgrade && (
+					Equals(lockMode, LockMode.UpgradeNoWait) || Equals(lockMode, LockMode.Force));
+				if (!isUpgrade && !isUpgradeNoWait)
+					return string.Empty;


Added shortcut. Previous code would have call Dialect.GetForUpdateString(lockMode) instead, but now, with Force handled, it should only yield string.Empty.

Thank you all for picking up and finish the work on this. I'm on a tight schedule and could not find the peace of mind to finalize it.

hazzik · 2018-03-13T21:51:39Z

src/NHibernate/Dialect/Dialect.cs

-		/// <value> True if the database supports <tt>FOR UPDATE OF</tt> syntax; false otherwise. </value>
+		/// <summary>Is <c>FOR UPDATE OF</c> syntax supported?</summary>
+		/// <value><see langword="true"/> if the database supports <c>FOR UPDATE OF</c> syntax; <see langword="false"/> otherwise. </value>
+		public virtual bool ForUpdateOf


SupportsForUpdateOf

hazzik · 2018-03-13T21:53:15Z

src/NHibernate/Dialect/Dialect.cs

+
+		/// <summary>Is <c>FOR UPDATE OF</c> syntax expecting columns?</summary>
+		/// <value><see langword="true"/> if the database expects a column list with <c>FOR UPDATE OF</c> syntax,
+		/// <see langword="false"/> if it expects table alias instead or do not support <c>FOR UPDATE OF</c> syntax.</value>
 		public virtual bool ForUpdateOfColumns


true/false semantic does not make sense, at least with the current name of the property. Let's rename it to RequiresColumnsForUpdateOf, or similar if you know a better name.

UsesColumnsWithForUpdateOf?

… too). Renaming an existing property without causing a breaking change causes some bloat.

Doan-Van-Tuan force-pushed the bugfix/1565-postgres-outer-join-entity-lock-issue branch from 9ff5f3a to d61cae6 Compare February 1, 2018 07:38

Doan-Van-Tuan closed this Feb 1, 2018

Doan-Van-Tuan reopened this Feb 1, 2018

Doan-Van-Tuan mentioned this pull request Feb 1, 2018

For update with outer join fails with PostgreSQL #1565

Closed

hazzik added the t: Fix label Feb 1, 2018

fredericDelaporte reviewed Feb 1, 2018

View reviewed changes

fredericDelaporte changed the title ~~GH1565 Add alias in "for update" clause to entity loading query conta…~~ For update with outer join fails with PostgreSQL Feb 1, 2018

fredericDelaporte reviewed Feb 1, 2018

View reviewed changes

Doan Van Tuan added 4 commits February 2, 2018 13:41

GH1565 Add alias in "for update" clause to entity loading query conta…

63e95cf

…ining outer join if database (e.g Postgres) does not support locking with outer join

Attemp to fix TC build by removing Configure() method from test

f5b9b19

Reformat file, replace spaces with tabs

abb3c82

GH1565 Refine code as per reviewer comments:

7effa35

- Restore and mark SetLockMode(LockMode) obsolete - Only check for outer join if the dialect is not support locking entire outer join - Rename test class to reflect that it not limited to PostgreSQL

Doan-Van-Tuan force-pushed the bugfix/1565-postgres-outer-join-entity-lock-issue branch from fc15b27 to 7effa35 Compare February 3, 2018 03:37

bahusoid reviewed Feb 3, 2018

View reviewed changes

hazzik changed the title ~~For update with outer join fails with PostgreSQL~~ WIP For update with outer join fails with PostgreSQL Feb 28, 2018

Merge branch 'master' into bugfix/1565-postgres-outer-join-entity-loc…

68f6b1d

…k-issue

hazzik requested a review from fredericDelaporte March 12, 2018 23:35

hazzik added 2 commits March 13, 2018 12:53

Adjust code as per review

0381456

Generate async code

15437c6

hazzik changed the title ~~WIP For update with outer join fails with PostgreSQL~~ Fix for update with outer join in PostgreSQL Mar 13, 2018

hazzik previously approved these changes Mar 13, 2018

View reviewed changes

Remove undue whitespace addition and do condition swap recommended by…

c65ab2b

… bahusoid.

fredericDelaporte dismissed hazzik’s stale review via c65ab2b March 13, 2018 11:16

Handle unimplemented/unsupported cases.

60f6002

Fix an undue reference equality check for lockmode.

133f244

fredericDelaporte force-pushed the bugfix/1565-postgres-outer-join-entity-lock-issue branch from d95a025 to 133f244 Compare March 13, 2018 11:59

Some more misleading xml comment fixes.

267977d

fredericDelaporte previously approved these changes Mar 13, 2018

View reviewed changes

hazzik reviewed Mar 13, 2018

View reviewed changes

fredericDelaporte added 2 commits March 14, 2018 01:32

Rename a new property (to be squashed, and all my other commits too).

d8f3449

Rename an existing property (to be squashed, and all my other commits…

d640fc9

… too). Renaming an existing property without causing a breaking change causes some bloat.

fredericDelaporte dismissed their stale review via d640fc9 March 14, 2018 00:49

hazzik approved these changes Mar 14, 2018

View reviewed changes

fredericDelaporte added this to the 5.1 milestone Mar 14, 2018

fredericDelaporte merged commit e38566d into nhibernate:master Mar 14, 2018

fredericDelaporte added the r: Fixed label Mar 14, 2018

Fix for update with outer join in PostgreSQL #1568

Fix for update with outer join in PostgreSQL #1568

Uh oh!

Conversation

Doan-Van-Tuan commented Feb 1, 2018 • edited by fredericDelaporte Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Doan-Van-Tuan commented Feb 1, 2018

Uh oh!

bahusoid commented Feb 1, 2018

Uh oh!

hazzik commented Feb 1, 2018

Uh oh!

fredericDelaporte commented Feb 1, 2018

Uh oh!

Doan-Van-Tuan commented Feb 1, 2018

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bahusoid Feb 3, 2018 • 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!

hazzik commented Mar 12, 2018

Uh oh!

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Mar 13, 2018 • 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!

fredericDelaporte Mar 14, 2018 • 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!

Uh oh!

Doan-Van-Tuan commented Feb 1, 2018 •

edited by fredericDelaporte

Loading

bahusoid Feb 3, 2018 •

edited

Loading

fredericDelaporte Mar 13, 2018 •

edited

Loading

fredericDelaporte Mar 14, 2018 •

edited

Loading