HHH-19937, HHH-19939 eliminate duplicate version check after refresh

hibernate-core/src/main/java/org/hibernate/LockOptions.java

@@ -308,8 +308,8 @@ public LockOptions setFollowOnStrategy(Locking.FollowOn followOnStrategy) {
 
 	public boolean hasNonDefaultOptions() {
 		return timeout != Timeouts.WAIT_FOREVER_MILLI
-				|| scope != Locking.Scope.ROOT_ONLY
-				|| followOnStrategy != Locking.FollowOn.ALLOW;
+			|| scope != Locking.Scope.ROOT_ONLY
+			|| followOnStrategy != Locking.FollowOn.ALLOW;
 	}
 
 
@@ -588,7 +588,7 @@ public LockMode findGreatestLockMode() {
 	 */
 	@Deprecated(since = "7", forRemoval = true)
 	public LockOptions makeCopy() {
-		final LockOptions copy = new LockOptions();
+		final var copy = new LockOptions();
 		copy( this, copy );
 		return copy;
 	}
@@ -606,7 +606,7 @@ public LockOptions makeDefensiveCopy() {
 			return this;
 		}
 		else {
-			final LockOptions copy = new LockOptions();
+			final var copy = new LockOptions();
 			copy( this, copy );
 			return copy;
 		}

hibernate-core/src/main/java/org/hibernate/event/internal/DefaultRefreshEventListener.java

@@ -6,6 +6,7 @@
 
 import org.hibernate.HibernateException;
 import org.hibernate.LockMode;
+import org.hibernate.LockOptions;
 import org.hibernate.NonUniqueObjectException;
 import org.hibernate.TransientObjectException;
 import org.hibernate.UnresolvableObjectException;
@@ -242,50 +243,55 @@ private static Object doRefresh(
 			LazyInitializer lazyInitializer,
 			Object id,
 			PersistenceContext persistenceContext) {
-		// Handle the requested lock-mode (if one) in relation to the entry's (if one) current lock-mode
-		var lockOptionsToUse = event.getLockOptions();
-		final LockMode requestedLockMode = lockOptionsToUse.getLockMode();
+		// Handle the requested lock mode, if any, in relation to the current
+		// lock mode, if any, of the entry.
+		final var lockOptions = event.getLockOptions();
+		final var requestedLockMode = lockOptions.getLockMode();
+		final LockOptions lockOptionsToUse;
 		final LockMode postRefreshLockMode;
-		if ( entry != null ) {
-			final LockMode currentLockMode = entry.getLockMode();
-			if ( currentLockMode.greaterThan( requestedLockMode ) ) {
-				// the requested lock-mode is less restrictive than the current one
-				//		- pass along the current lock-mode (after accounting for WRITE)
-				lockOptionsToUse = lockOptionsToUse.makeCopy();
-				if ( currentLockMode == LockMode.WRITE
-						|| currentLockMode == LockMode.PESSIMISTIC_WRITE
-						|| currentLockMode == LockMode.PESSIMISTIC_READ ) {
-					// our transaction should already hold the exclusive lock on
-					// the underlying row - so READ should be sufficient.
-					//
-					// in fact, this really holds true for any current lock-mode that indicates we
-					// hold an exclusive lock on the underlying row - but we *need* to handle
-					// WRITE specially because the Loader/Locker mechanism does not allow for WRITE
-					// locks
-					lockOptionsToUse.setLockMode( LockMode.READ );
-					// and prepare to reset the entry lock-mode to the previous lock mode after
-					// the refresh completes
-					postRefreshLockMode = currentLockMode;
-				}
-				else {
-					lockOptionsToUse.setLockMode( currentLockMode );
-					postRefreshLockMode = null;
-				}
+		if ( entry == null ) {
+			// Should never happen now that we can't refresh detached entities.
+			lockOptionsToUse = lockOptions;
+			postRefreshLockMode = null;
+		}
+		else {
+			final var currentLockMode = entry.getLockMode();
+			if ( requestedLockMode.greaterThan( currentLockMode )
+					|| currentLockMode == LockMode.NONE
+					|| currentLockMode == LockMode.READ ) {
+				// Either the current transaction does not hold an exclusive
+				// lock or we're upgrading to a more restrictive lock mode.
+				// Nothing special to do in this case.
+				lockOptionsToUse = lockOptions;
+				postRefreshLockMode = null;
 			}
 			else {
-				postRefreshLockMode = null;
+				// The requested lock mode is no more restrictive than the
+				// exclusive lock already held. Preserve the current mode.
+				lockOptionsToUse = lockOptions.makeCopy();
+				// The current transaction already holds an exclusive lock,
+				// so refreshing with READ is sufficient. Also see HHH-19937;
+				// we want to avoid dupe version checks.
+				lockOptionsToUse.setLockMode( LockMode.READ );
+				// But prepare to reset the entry lock mode to the previous
+				// lock mode after the refresh completes, except in the case
+				// where the two lock modes belong to the same level, in which
+				// case we will need to reset the entry to the requested mode.
+				postRefreshLockMode =
+						requestedLockMode.lessThan( currentLockMode )
+								? currentLockMode
+								: requestedLockMode;
 			}
 		}
-		else {
-			postRefreshLockMode = null;
-		}
 
+		// Go ahead and reload the entity from the database.
 		final Object result = persister.load( id, object, lockOptionsToUse, source );
 		if ( result != null ) {
-			// apply postRefreshLockMode, if needed
+			// Apply the preserved postRefreshLockMode, if any.
 			if ( postRefreshLockMode != null ) {
-				// if we get here, there was a previous entry, and we need to reset its lock mode
-				//		- however, the refresh operation actually creates a new entry, so get it
+				// If we get here, there was a previous entry, and a lock was already held.
+				// But the refresh operation created a new entry with a less restrictive
+				// lock mode recorded. So we need to reset the lock mode of the new entry.
 				persistenceContext.getEntry( result ).setLockMode( postRefreshLockMode );
 			}
 			source.setReadOnly( result, isReadOnly( entry, persister, lazyInitializer, source ) );

hibernate-core/src/main/java/org/hibernate/loader/ast/internal/SingleIdEntityLoaderStandardImpl.java

@@ -11,7 +11,6 @@
 import org.hibernate.LockMode;
 import org.hibernate.LockOptions;
 import org.hibernate.engine.spi.LoadQueryInfluencers;
-import org.hibernate.engine.spi.SessionFactoryImplementor;
 import org.hibernate.engine.spi.SharedSessionContractImplementor;
 import org.hibernate.loader.ast.spi.CascadingFetchProfile;
 import org.hibernate.metamodel.mapping.EntityMappingType;
@@ -24,17 +23,18 @@
  */
 public class SingleIdEntityLoaderStandardImpl<T> extends SingleIdEntityLoaderSupport<T> {
 
-	private final EnumMap<LockMode, SingleIdLoadPlan<T>> selectByLockMode = new EnumMap<>( LockMode.class );
-	private EnumMap<CascadingFetchProfile, SingleIdLoadPlan<T>> selectByInternalCascadeProfile;
+	private final EnumMap<LockMode, SingleIdLoadPlan<T>> selectByLockMode =
+			new EnumMap<>( LockMode.class );
+	private final EnumMap<CascadingFetchProfile, EnumMap<LockMode,SingleIdLoadPlan<T>>> selectByInternalCascadeProfile =
+			new EnumMap<>( CascadingFetchProfile.class );
 
 	private final BiFunction<LockOptions, LoadQueryInfluencers, SingleIdLoadPlan<T>> loadPlanCreator;
 
 	public SingleIdEntityLoaderStandardImpl(
 			EntityMappingType entityDescriptor,
 			LoadQueryInfluencers loadQueryInfluencers) {
 		this( entityDescriptor, loadQueryInfluencers,
-				(lockOptions, influencers) ->
-						createLoadPlan( entityDescriptor, lockOptions, influencers, influencers.getSessionFactory() ) );
+				(lockOptions, influencers) -> createLoadPlan( entityDescriptor, lockOptions, influencers) );
 	}
 
 	/**
@@ -50,13 +50,11 @@ protected SingleIdEntityLoaderStandardImpl(
 		// todo (6.0) : consider creating a base AST and "cloning" it
 		super( entityDescriptor, influencers.getSessionFactory() );
 		this.loadPlanCreator = loadPlanCreator;
-		// see org.hibernate.persister.entity.AbstractEntityPersister#createLoaders
-		// we should preload a few - maybe LockMode.NONE and LockMode.READ
-		final var noLocking = new LockOptions();
-		final var singleIdLoadPlan = loadPlanCreator.apply( noLocking, influencers );
-		if ( isLoadPlanReusable( noLocking, influencers ) ) {
-			selectByLockMode.put( LockMode.NONE, singleIdLoadPlan );
-		}
+		// Preload some load plans (for now only do it for LockMode.NONE)
+		final var singleIdLoadPlan = loadPlanCreator.apply( LockOptions.NONE, influencers );
+//		if ( isLoadPlanReusable( LockOptions.NONE, influencers ) ) {
+		selectByLockMode.put( LockMode.NONE, singleIdLoadPlan );
+//		}
 	}
 
 	@Override
@@ -114,38 +112,60 @@ private SingleIdLoadPlan<T> getRegularLoadPlan(LockOptions lockOptions, LoadQuer
 	}
 
 	private SingleIdLoadPlan<T> getInternalCascadeLoadPlan(LockOptions lockOptions, LoadQueryInfluencers influencers) {
-		final var fetchProfile = influencers.getEnabledCascadingFetchProfile();
-		if ( selectByInternalCascadeProfile == null ) {
-			selectByInternalCascadeProfile = new EnumMap<>( CascadingFetchProfile.class );
+		// TODO: It might be more efficient to just instantiate a LoadPlanKey
+		//       object here than it is to maintain an EnumMap of EnumMaps
+		final var lockMode = lockOptions.getLockMode();
+		EnumMap<LockMode,SingleIdLoadPlan<T>> map;
+		if ( isLoadPlanReusable( lockOptions, influencers ) ) {
+			final var fetchProfile = influencers.getEnabledCascadingFetchProfile();
+			final var existingMap = selectByInternalCascadeProfile.get( fetchProfile );
+			if ( existingMap == null ) {
+				map = new EnumMap<>( LockMode.class );
+				selectByInternalCascadeProfile.put( fetchProfile, map );
+			}
+			else {
+				final var existing = existingMap.get( lockMode );
+				if ( existing != null ) {
+					return existing;
+				}
+				else {
+					map = existingMap;
+				}
+			}
 		}
 		else {
-			final var existing = selectByInternalCascadeProfile.get( fetchProfile );
-			if ( existing != null ) {
-				return existing;
-			}
+			map = null;
 		}
+
 		final var plan = loadPlanCreator.apply( lockOptions, influencers );
-		selectByInternalCascadeProfile.put( fetchProfile, plan );
+		if ( map != null ) {
+			map.put( lockMode, plan );
+		}
 		return plan;
 	}
 
+	/**
+	 * We key the caches only by {@link LockMode} and {@link CascadingFetchProfile}.
+	 * If there is a pessimistic lock with non-default options like timeout, a custom
+	 * fetch profile, or an entity graph, we don't cache and reuse the plan.
+	 */
 	private boolean isLoadPlanReusable(LockOptions lockOptions, LoadQueryInfluencers influencers) {
 		if ( lockOptions.getLockMode().isPessimistic() && lockOptions.hasNonDefaultOptions() ) {
 			return false;
 		}
 		else {
-			return !getLoadable().isAffectedByEntityGraph( influencers )
-				&& !getLoadable().isAffectedByEnabledFetchProfiles( influencers );
+			final var loadable = getLoadable();
+			return !loadable.isAffectedByEntityGraph( influencers )
+				&& !loadable.isAffectedByEnabledFetchProfiles( influencers );
 		}
 	}
 
 	private static <T> SingleIdLoadPlan<T> createLoadPlan(
 			EntityMappingType loadable,
 			LockOptions lockOptions,
-			LoadQueryInfluencers influencers,
-			SessionFactoryImplementor factory) {
-
+			LoadQueryInfluencers influencers) {
 		final var jdbcParametersBuilder = JdbcParametersList.newBuilder();
+		final var factory = influencers.getSessionFactory();
 		return new SingleIdLoadPlan<>(
 				loadable,
 				loadable.getIdentifierMapping(),

hibernate-core/src/main/java/org/hibernate/loader/ast/internal/SingleIdLoadPlan.java

@@ -5,8 +5,6 @@
 package org.hibernate.loader.ast.internal;
 
 import org.hibernate.LockOptions;
-import org.hibernate.engine.jdbc.env.spi.JdbcEnvironment;
-import org.hibernate.engine.jdbc.spi.JdbcServices;
 import org.hibernate.engine.spi.SessionFactoryImplementor;
 import org.hibernate.engine.spi.SharedSessionContractImplementor;
 import org.hibernate.loader.ast.spi.Loadable;
@@ -15,7 +13,6 @@
 import org.hibernate.query.internal.SimpleQueryOptions;
 import org.hibernate.query.spi.QueryOptions;
 import org.hibernate.query.spi.QueryOptionsAdapter;
-import org.hibernate.sql.ast.SqlAstTranslatorFactory;
 import org.hibernate.sql.ast.tree.select.SelectStatement;
 import org.hibernate.sql.exec.internal.BaseExecutionContext;
 import org.hibernate.sql.exec.internal.CallbackImpl;
@@ -54,21 +51,21 @@ public SingleIdLoadPlan(
 			SessionFactoryImplementor sessionFactory) {
 		this.entityMappingType = entityMappingType;
 		this.restrictivePart = restrictivePart;
-		this.lockOptions = lockOptions.makeCopy();
+		this.lockOptions = lockOptions.makeDefensiveCopy();
 		this.jdbcParameters = jdbcParameters;
-		final JdbcServices jdbcServices = sessionFactory.getJdbcServices();
-		final JdbcEnvironment jdbcEnvironment = jdbcServices.getJdbcEnvironment();
-		final SqlAstTranslatorFactory sqlAstTranslatorFactory = jdbcEnvironment.getSqlAstTranslatorFactory();
-		this.jdbcSelect = sqlAstTranslatorFactory.buildSelectTranslator( sessionFactory, sqlAst )
-				.translate(
-						null,
-						new QueryOptionsAdapter() {
-							@Override
-							public LockOptions getLockOptions() {
-								return lockOptions;
-							}
-						}
-				);
+		this.jdbcSelect =
+				sessionFactory.getJdbcServices().getJdbcEnvironment()
+						.getSqlAstTranslatorFactory()
+						.buildSelectTranslator( sessionFactory, sqlAst )
+						.translate(
+								null,
+								new QueryOptionsAdapter() {
+									@Override
+									public LockOptions getLockOptions() {
+										return lockOptions;
+									}
+								}
+						);
 	}
 
 	protected LockOptions getLockOptions() {

hibernate-core/src/test/java/org/hibernate/orm/test/locking/C.java

@@ -0,0 +1,60 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ * Copyright Red Hat Inc. and Hibernate Authors
+ */
+package org.hibernate.orm.test.locking;
+
+import jakarta.persistence.Column;
+import jakarta.persistence.Entity;
+import jakarta.persistence.GeneratedValue;
+import jakarta.persistence.Id;
+import jakarta.persistence.Table;
+import jakarta.persistence.Version;
+import org.hibernate.annotations.GenericGenerator;
+
+/**
+ * @author Steve Ebersole
+ */
+@Entity
+@Table( name = "T_LOCK_C" )
+public class C {
+	private Long id;
+	private String value;
+	private int version;
+
+	public C() {
+	}
+
+	public C(String value) {
+		this.value = value;
+	}
+
+	@Id
+	@GeneratedValue( generator = "increment" )
+	@GenericGenerator( name = "increment", strategy = "increment" )
+	public Long getId() {
+		return id;
+	}
+
+	public void setId(Long id) {
+		this.id = id;
+	}
+
+	@Column(name="c_value")
+	public String getValue() {
+		return value;
+	}
+
+	public void setValue(String value) {
+		this.value = value;
+	}
+
+	@Version
+	public int getVersion() {
+		return version;
+	}
+
+	public void setVersion(int version) {
+		this.version = version;
+	}
+}