Skip to content

Conversation

@sebersole
Copy link
Member

@sebersole sebersole commented Nov 13, 2025

HHH-19925 - Locking root(s) should be based on select-clause, not from-clause

DO NOT MERGE - WIP


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.


https://hibernate.atlassian.net/browse/HHH-19925

@sebersole
Copy link
Member Author

At the moment, this just handles the collection of "root" selections which are entities[1] and exposes them from the SelectStatement SQL AST. Next I'll need to adjust the SqlAstTranslator to use these NavigablePaths rather than the NavigablePaths from the FromClause, which should not be difficult.

[1] It does not yet cover entity selectiions as part of dynamic instantiation...


private static <I> JdbcParameterBindings buildJdbcParameterBindings(
EntityIdentifierMapping identifierMapping,
List<I> ids,

Check notice

Code scanning / CodeQL

Useless parameter Note test

The parameter 'ids' is never used.
@sebersole
Copy link
Member Author

sebersole commented Nov 13, 2025

Second push now collects "root paths for locking" for both (1) entity selections and (2) path selections rooted at an entity, as well as either occuring as an argument in dynamic instantiation.

Will work on tying this in with the previous pessimistic locking work.

}
}

private void collectRootPathsForLocking(SqmPath<?> selectedPath) {

Check notice

Code scanning / CodeQL

Confusing overloading of methods Note

Method BaseSqmToSqlAstConverter.collectRootPathsForLocking(..) could be confused with overloaded method
collectRootPathsForLocking
, since dispatch depends on static types.
}
}

private void collectRootPathsForLocking(SqmDynamicInstantiation<?> dynamicInstantiation) {

Check notice

Code scanning / CodeQL

Confusing overloading of methods Note

Method BaseSqmToSqlAstConverter.collectRootPathsForLocking(..) could be confused with overloaded method
collectRootPathsForLocking
, since dispatch depends on static types.
@gavinking
Copy link
Member

Second push now collects "root paths for locking" for both (1) entity selections and (2) path selections rooted at an entity,

Nice.

@sebersole sebersole marked this pull request as ready for review November 14, 2025 14:51
@sebersole
Copy link
Member Author

With this 3rd push, this now works purely in terms of the singular expectation from the Jira. I have a feeling other tests may fail with no-longer-accurate assumptions on some databases, so let's see what CI says.

In terms of implementation, I was able to make this pretty low impact which I was happy with. I had to change to make QuerySpec be the thing that carries around the "root paths for locking" to make it easier to integrate into the SQL AST translation, which is fine; but a little leery that you need to add these iteratively to the QuerySpec (which are generally built eagerly) as opposed to passing as ctor args - just means it is eassier to miss in some code paths.

Comment on lines +2219 to +2220
rootPathsForLockingCollector = (path) ->
currentQuerySpec().applyRootPathForLocking( path );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me as if the current query spec could change if we enter a subquery in the select clause. I guess that is not intended, so here is as suggestion that would also look simpler.

Suggested change
rootPathsForLockingCollector = (path) ->
currentQuerySpec().applyRootPathForLocking( path );
rootPathsForLockingCollector = currentQuerySpec()::applyRootPathForLocking;

Copy link
Member

@beikov beikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than what I found this looks good to me. It seems like QuerySpec#rootPathsForLocking represents the for update clause, so maybe it's worth naming it that way? Just a thought.

@sebersole
Copy link
Member Author

Other than what I found this looks good to me. It seems like QuerySpec#rootPathsForLocking represents the for update clause, so maybe it's worth naming it that way? Just a thought.

No, that's not quite correct. Those are the NavigablePaths for model-parts which are "root selections". Per the associatyed Jira, the change here is to base "roots" for locking on the select-clause rather than the from-clause.

@sebersole sebersole merged commit 8822816 into hibernate:main Nov 19, 2025
28 checks passed
@sebersole sebersole deleted the locking-root-select branch November 19, 2025 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants