Skip to content

Commit 28e0421

Browse files
sebersolesebersole
committed
HHH-19925 - Locking root(s) should be based on select-clause, not from-clause
1 parent d3b777f commit 28e0421

File tree

8 files changed

+229
-124
lines changed

8 files changed

+229
-124
lines changed

‎hibernate-core/src/main/java/org/hibernate/dialect/AbstractTransactSQLDialect.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import org.hibernate.query.sqm.mutation.spi.SqmMultiTableInsertStrategy;
2828
import org.hibernate.query.sqm.mutation.spi.SqmMultiTableMutationStrategy;
2929
import org.hibernate.sql.ast.SqlAstNodeRenderingMode;
30+
import org.hibernate.sql.ast.internal.TransactSQLLockingClauseStrategy;
3031
import org.hibernate.sql.ast.spi.LockingClauseStrategy;
3132
import org.hibernate.sql.ast.spi.SqlAppender;
3233
import org.hibernate.sql.ast.tree.select.QuerySpec;
@@ -40,7 +41,6 @@
4041
import java.sql.Types;
4142
import java.util.Map;
4243

43-
import static org.hibernate.sql.ast.internal.NonLockingClauseStrategy.NON_CLAUSE_STRATEGY;
4444
import static org.hibernate.type.SqlTypes.BLOB;
4545
import static org.hibernate.type.SqlTypes.BOOLEAN;
4646
import static org.hibernate.type.SqlTypes.CLOB;
@@ -200,8 +200,7 @@ public boolean qualifyIndexName() {
200200

201201
@Override
202202
public LockingClauseStrategy getLockingClauseStrategy(QuerySpec querySpec, LockOptions lockOptions) {
203-
// T-SQL uses table-based lock hints and thus does not support FOR UPDATE clause
204-
return NON_CLAUSE_STRATEGY;
203+
return new TransactSQLLockingClauseStrategy( lockOptions.getScope(), querySpec.getRootPathsForLocking() );
205204
}
206205

207206
@Override

‎hibernate-core/src/main/java/org/hibernate/dialect/sql/ast/SQLServerSqlAstTranslator.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,9 @@ public void renderNamedSetReturningFunction(String functionName, List<? extends
202202
@Override
203203
protected boolean renderNamedTableReference(NamedTableReference tableReference, LockMode lockMode) {
204204
final String tableExpression = tableReference.getTableExpression();
205-
if ( tableReference instanceof UnionTableReference && lockMode != LockMode.NONE && tableExpression.charAt( 0 ) == '(' ) {
205+
if ( tableReference instanceof UnionTableReference
206+
&& lockMode != LockMode.NONE
207+
&& tableExpression.charAt( 0 ) == '(' ) {
206208
// SQL Server requires to push down the lock hint to the actual table names
207209
int searchIndex = 0;
208210
int unionIndex;

‎hibernate-core/src/main/java/org/hibernate/sql/ast/internal/AbstractLockingClauseStrategy.java‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
* Copyright Red Hat Inc. and Hibernate Authors
4+
*/
5+
package org.hibernate.sql.ast.internal;
6+
7+
import org.hibernate.Locking;
8+
import org.hibernate.metamodel.mapping.PluralAttributeMapping;
9+
import org.hibernate.metamodel.mapping.internal.BasicValuedCollectionPart;
10+
import org.hibernate.spi.NavigablePath;
11+
import org.hibernate.sql.ast.spi.LockingClauseStrategy;
12+
import org.hibernate.sql.ast.tree.from.TableGroup;
13+
import org.hibernate.sql.ast.tree.from.TableGroupJoin;
14+
15+
import java.util.HashSet;
16+
import java.util.Set;
17+
18+
/**
19+
* @author Steve Ebersole
20+
*/
21+
public abstract class AbstractLockingClauseStrategy implements LockingClauseStrategy {
22+
private final Locking.Scope lockingScope;
23+
private final Set<NavigablePath> rootsForLocking;
24+
25+
private Set<NavigablePath> pathsToLock;
26+
27+
public AbstractLockingClauseStrategy(
28+
Locking.Scope lockingScope,
29+
Set<NavigablePath> rootsForLocking) {
30+
this.lockingScope = lockingScope;
31+
this.rootsForLocking = rootsForLocking == null ? Set.of() : rootsForLocking;
32+
}
33+
34+
protected Locking.Scope getLockingScope() {
35+
return lockingScope;
36+
}
37+
38+
protected Set<NavigablePath> getRootsForLocking() {
39+
return rootsForLocking;
40+
}
41+
42+
protected Set<NavigablePath> getPathsToLock() {
43+
return pathsToLock;
44+
}
45+
46+
@Override
47+
public boolean shouldLockRoot(TableGroup root) {
48+
return rootsForLocking.contains( root.getNavigablePath() );
49+
}
50+
51+
@Override
52+
public boolean shouldLockJoin(TableGroup joinedGroup) {
53+
// we only want to consider applying locks to joins in 2 cases:
54+
// 1) It is a root path for locking (aka occurs in the domain select-clause)
55+
// 2) It's left-hand side is to be locked
56+
if ( isRootForLocking( joinedGroup ) ) {
57+
return true;
58+
}
59+
else if ( isLhsLocked( joinedGroup ) ) {
60+
if ( lockingScope == Locking.Scope.INCLUDE_COLLECTIONS ) {
61+
// if the TableGroup is an owned (aka, non-inverse) collection,
62+
// and we are to lock collections, track it
63+
if ( joinedGroup.getModelPart() instanceof PluralAttributeMapping attrMapping ) {
64+
if ( !attrMapping.getCollectionDescriptor().isInverse() ) {
65+
// owned collection element-collection
66+
return attrMapping.getElementDescriptor() instanceof BasicValuedCollectionPart;
67+
}
68+
}
69+
}
70+
else if ( lockingScope == Locking.Scope.INCLUDE_FETCHES ) {
71+
return joinedGroup.isFetched();
72+
}
73+
}
74+
75+
return false;
76+
}
77+
78+
protected boolean isRootForLocking(TableGroup joinedGroup) {
79+
return rootsForLocking.contains( joinedGroup.getNavigablePath() );
80+
}
81+
82+
protected boolean isLhsLocked(TableGroup joinedGroup) {
83+
// todo (pessimistic-locking) : The use of NavigablePath#parent for LHS here is not ideal.
84+
// However, the only alternative is to change the method signature to pass the
85+
// join's LHS which would have a broad impact on Dialects and translators.
86+
// I'm sure this will miss some cases, but let's start here fow now and deal with
87+
// these other cases as they come up.
88+
return pathsToLock != null
89+
&& pathsToLock.contains( joinedGroup.getNavigablePath().getParent() );
90+
}
91+
92+
@Override
93+
public void registerRoot(TableGroup root) {
94+
if ( shouldLockRoot( root ) ) {
95+
trackRoot( root );
96+
}
97+
}
98+
99+
protected void trackRoot(TableGroup root) {
100+
if ( pathsToLock == null ) {
101+
pathsToLock = new HashSet<>();
102+
}
103+
pathsToLock.add( root.getNavigablePath() );
104+
}
105+
106+
@Override
107+
public void registerJoin(TableGroupJoin join) {
108+
if ( shouldLockJoin( join.getJoinedGroup() ) ) {
109+
trackJoin( join );
110+
}
111+
}
112+
113+
protected void trackJoin(TableGroupJoin join) {
114+
if ( pathsToLock == null ) {
115+
pathsToLock = new HashSet<>();
116+
}
117+
pathsToLock.add( join.getNavigablePath() );
118+
}
119+
}

‎hibernate-core/src/main/java/org/hibernate/sql/ast/internal/NonLockingClauseStrategy.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,16 @@
2222
public class NonLockingClauseStrategy implements LockingClauseStrategy {
2323
public static final NonLockingClauseStrategy NON_CLAUSE_STRATEGY = new NonLockingClauseStrategy();
2424

25+
@Override
26+
public boolean shouldLockRoot(TableGroup root) {
27+
return false;
28+
}
29+
30+
@Override
31+
public boolean shouldLockJoin(TableGroup joinedGroup) {
32+
return false;
33+
}
34+
2535
@Override
2636
public void registerRoot(TableGroup root) {
2737
// nothing to do

‎hibernate-core/src/main/java/org/hibernate/sql/ast/internal/StandardLockingClauseStrategy.java‎

Lines changed: 32 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
import jakarta.persistence.Timeout;
88
import org.hibernate.AssertionFailure;
99
import org.hibernate.LockOptions;
10-
import org.hibernate.Locking;
1110
import org.hibernate.dialect.Dialect;
1211
import org.hibernate.dialect.RowLockStrategy;
1312
import org.hibernate.internal.util.collections.CollectionHelper;
@@ -17,12 +16,10 @@
1716
import org.hibernate.metamodel.mapping.PluralAttributeMapping;
1817
import org.hibernate.metamodel.mapping.SelectableMappings;
1918
import org.hibernate.metamodel.mapping.TableDetails;
20-
import org.hibernate.metamodel.mapping.internal.BasicValuedCollectionPart;
2119
import org.hibernate.persister.entity.EntityPersister;
2220
import org.hibernate.persister.entity.mutation.EntityTableMapping;
2321
import org.hibernate.spi.NavigablePath;
2422
import org.hibernate.sql.ast.SqlAstJoinType;
25-
import org.hibernate.sql.ast.spi.LockingClauseStrategy;
2623
import org.hibernate.sql.ast.spi.SqlAppender;
2724
import org.hibernate.sql.ast.tree.from.NamedTableReference;
2825
import org.hibernate.sql.ast.tree.from.TableGroup;
@@ -44,47 +41,41 @@
4441
*
4542
* @author Steve Ebersole
4643
*/
47-
public class StandardLockingClauseStrategy implements LockingClauseStrategy {
44+
public class StandardLockingClauseStrategy extends AbstractLockingClauseStrategy {
4845
private final Dialect dialect;
4946
private final RowLockStrategy rowLockStrategy;
5047
private final PessimisticLockKind lockKind;
51-
private final Locking.Scope lockingScope;
5248
private final Timeout timeout;
5349

54-
/**
55-
* @implNote Tracked separately from {@linkplain #rootsToLock} and
56-
* {@linkplain #joinsToLock} to help answer {@linkplain #containsOuterJoins()}
57-
* for {@linkplain RowLockStrategy#NONE cases} where we otherwise don't need to
58-
* track the tables, allowing to avoid the overhead of the Sets. There is a
59-
* slight trade-off in that we need to inspect the from-elements to make that
60-
* determination when we might otherwise not need to - memory versus cpu.
61-
*/
6250
private boolean queryHasOuterJoins = false;
6351

64-
private final Set<NavigablePath> rootsForLocking;
65-
6652
private Set<TableGroup> rootsToLock;
6753
private Set<TableGroupJoin> joinsToLock;
68-
private Set<NavigablePath> pathsToLock;
6954

7055
public StandardLockingClauseStrategy(
7156
Dialect dialect,
7257
PessimisticLockKind lockKind,
7358
RowLockStrategy rowLockStrategy,
7459
LockOptions lockOptions,
7560
Set<NavigablePath> rootsForLocking) {
76-
// NOTE: previous versions would limit collection based on RowLockStrategy.
77-
// however, this causes problems with the new follow-on locking approach
61+
super( lockOptions.getScope(), rootsForLocking );
7862

7963
assert lockKind != PessimisticLockKind.NONE;
8064

8165
this.dialect = dialect;
8266
this.rowLockStrategy = rowLockStrategy;
8367
this.lockKind = lockKind;
84-
this.lockingScope = lockOptions.getScope();
8568
this.timeout = lockOptions.getTimeout();
69+
}
70+
71+
@Override
72+
public Collection<TableGroup> getRootsToLock() {
73+
return rootsToLock;
74+
}
8675

87-
this.rootsForLocking = rootsForLocking == null ? Set.of() : rootsForLocking;
76+
@Override
77+
public Collection<TableGroupJoin> getJoinsToLock() {
78+
return joinsToLock;
8879
}
8980

9081
@Override
@@ -96,62 +87,31 @@ public void registerRoot(TableGroup root) {
9687
}
9788
}
9889

99-
if ( rootsForLocking.contains( root.getNavigablePath() ) ) {
100-
if ( rootsToLock == null ) {
101-
rootsToLock = new HashSet<>();
102-
}
103-
if ( pathsToLock == null ) {
104-
pathsToLock = new HashSet<>();
105-
}
106-
rootsToLock.add( root );
107-
pathsToLock.add( root.getNavigablePath() );
108-
}
90+
super.registerRoot( root );
10991
}
11092

11193
@Override
112-
public void registerJoin(TableGroupJoin join) {
113-
checkForOuterJoins( join );
114-
115-
// we only want to consider applying locks to joins in 2 cases:
116-
// 1) It is a root path for locking (aka occurs in the domain select-clause)
117-
// 2) It's left-hand side is to be locked
118-
if ( isRootForLocking( join ) ) {
119-
trackJoin( join );
120-
}
121-
else if ( isLhsLocked( join ) ) {
122-
if ( lockingScope == Locking.Scope.INCLUDE_COLLECTIONS ) {
123-
// if the TableGroup is an owned (aka, non-inverse) collection,
124-
// and we are to lock collections, track it
125-
if ( join.getJoinedGroup().getModelPart() instanceof PluralAttributeMapping attrMapping ) {
126-
if ( !attrMapping.getCollectionDescriptor().isInverse() ) {
127-
// owned collection
128-
if ( attrMapping.getElementDescriptor() instanceof BasicValuedCollectionPart ) {
129-
// an element-collection
130-
trackJoin( join );
131-
}
132-
}
133-
}
134-
}
135-
else if ( lockingScope == Locking.Scope.INCLUDE_FETCHES ) {
136-
if ( join.getJoinedGroup().isFetched() ) {
137-
trackJoin( join );
138-
}
139-
}
94+
protected void trackRoot(TableGroup root) {
95+
super.trackRoot( root );
96+
if ( rootsToLock == null ) {
97+
rootsToLock = new HashSet<>();
14098
}
99+
rootsToLock.add( root );
141100
}
142101

143-
private boolean isRootForLocking(TableGroupJoin join) {
144-
return rootsForLocking.contains( join.getNavigablePath() );
102+
@Override
103+
public void registerJoin(TableGroupJoin join) {
104+
checkForOuterJoins( join );
105+
super.registerJoin( join );
145106
}
146107

147-
private boolean isLhsLocked(TableGroupJoin join) {
148-
// TODO (pessimistic-locking) : The use of NavigablePath#parent for LHS here is not ideal.
149-
// However, the only alternative is to change the method signature to pass the
150-
// join's LHS which would have a broad impact on Dialects and translators.
151-
// I'm sure this will miss some cases, but let's start here fow now and deal with
152-
// these other cases as they come up.
153-
return pathsToLock != null
154-
&& pathsToLock.contains( join.getNavigablePath().getParent() );
108+
@Override
109+
protected void trackJoin(TableGroupJoin join) {
110+
super.trackJoin( join );
111+
if ( joinsToLock == null ) {
112+
joinsToLock = new LinkedHashSet<>();
113+
}
114+
joinsToLock.add( join );
155115
}
156116

157117
private void checkForOuterJoins(TableGroupJoin join) {
@@ -179,17 +139,6 @@ private boolean hasOuterJoin(TableGroupJoin join) {
179139
return false;
180140
}
181141

182-
private void trackJoin(TableGroupJoin join) {
183-
if ( joinsToLock == null ) {
184-
joinsToLock = new LinkedHashSet<>();
185-
}
186-
if ( pathsToLock == null ) {
187-
pathsToLock = new HashSet<>();
188-
}
189-
joinsToLock.add( join );
190-
pathsToLock.add( join.getNavigablePath() );
191-
}
192-
193142
@Override
194143
public boolean containsOuterJoins() {
195144
return queryHasOuterJoins;
@@ -201,16 +150,6 @@ public void render(SqlAppender sqlAppender) {
201150
renderResultSetOptions( sqlAppender );
202151
}
203152

204-
@Override
205-
public Collection<TableGroup> getRootsToLock() {
206-
return rootsToLock;
207-
}
208-
209-
@Override
210-
public Collection<TableGroupJoin> getJoinsToLock() {
211-
return joinsToLock;
212-
}
213-
214153
protected void renderLockFragment(SqlAppender sqlAppender) {
215154
final String fragment;
216155
if ( rowLockStrategy == RowLockStrategy.NONE ) {
@@ -233,13 +172,13 @@ private String collectLockItems() {
233172
}
234173

235174
final List<String> lockItems = new ArrayList<>();
236-
if ( rootsToLock != null ) {
237-
for ( TableGroup root : rootsToLock ) {
175+
if ( getRootsToLock() != null ) {
176+
for ( TableGroup root : getRootsToLock() ) {
238177
collectLockItems( root, lockItems );
239178
}
240179
}
241-
if ( joinsToLock != null ) {
242-
for ( TableGroupJoin join : joinsToLock ) {
180+
if ( getJoinsToLock() != null ) {
181+
for ( TableGroupJoin join : getJoinsToLock() ) {
243182
collectLockItems( join.getJoinedGroup(), lockItems );
244183
}
245184
}

0 commit comments

Comments
 (0)