Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propagate read-only status to Hibernate Session through setDefaultReadOnly [SPR-16956] #21494

Closed
spring-issuemaster opened this issue Jun 19, 2018 · 10 comments

Comments

@spring-issuemaster
Copy link
Collaborator

commented Jun 19, 2018

MIhalcea Vlad opened SPR-16956 and commented

By default,  @Transactional(readOnly = true) sets the FlushMode to MANUAL. However, Hibernate can save up lots of memory by discarding the associated hydrated state if we also set the session.setDefaultReadOnly(true).

Not only that we save memory, but we also save CPU cycles because, if the user tries to do a manual flush, we won't propagate it to any entity since they are virtually read-only.


Affects: 5.0.7

Issue Links:

  • #20850 Support for Hibernate ORM 5.3
  • #20852 Support Hibernate 5.3's ManagedBeanRegistry for dependency injection
  • #20314 Propagate read-only status as FlushMode.MANUAL to Query instances
  • #22089 HibernateTransactionManager (unintentionally) bound to Hibernate 5.2 SharedSessionContractImplementor
  • #21540 LocalSessionFactoryBean and HibernateTransactionManager for JPA EntityManagerFactory setup
  • #21749 HibernateTransactionManager should lazily acquire JDBC Connection (like HibernateJpaDialect)
  • #21553 HibernateJpaDialect cannot translate JDBCException to custom DataAccessException

Referenced from: pull request #1861, and commits 010ba33, d22d408

0 votes, 5 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 19, 2018

MIhalcea Vlad commented

Pull Request: #1861

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 20, 2018

Juergen Hoeller commented

MIhalcea Vlad, does it make sense to also set each Query to FlushMode.MANUAL in such a scenario, as suggested by #20314? Or is this effectively covered by Session.setDefaultReadOnly(true) already?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 3, 2018

Juergen Hoeller commented

I've rolled this into HibernateTransactionManager in a slightly revision fashion, with no need to store the previous read-only setting at Session level since we're only applying it to a new Session which we're immediately closing afterwards anyway. This should also cover the intent of #20314, avoiding any dirty checking overhead at query level.

For the time being, I'd prefer leaving this as a Hibernate-native feature. After all, as of Hibernate 5.2, LocalSessionFactoryBean can even be used for a JPA setup since the SessionFactory exposes itself as an EntityManagerFactory directly. This allows us to put some features only there, with HibernateJpaDialect providing less impactful default adaptations with a focus on matching EclipseLinkJpaDialect (more so than on exposing Hibernate specifics). Our flush mode handling is an easier pill to swallow there since it is an extension of a JPA concept and just suppresses default flushes but doesn't impact the ability for custom flush attempts.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 4, 2018

MIhalcea Vlad commented

Hi, Jurgen.

I've just seen your question. I would not set the flush mode to MANUAL as that could prevent the Persistence Context to flush prior to executing the query and break read-your-own-write consistency issues.

Related to your changes, does Spring Boot use the HibernateTransactionManager or it always picks the JpaTRanscationMnagaer no matter the underlying JPA DIalect?

If Spring Boot always picks the JpaTranscationManager, then this fix will not have a significant impact.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 5, 2018

Juergen Hoeller commented

I've added corresponding behavior to HibernateJpaDialect as well, based on a specific marker on the passed-in TransactionDefinition which indicates a transactional-local resource (analogous to the check in HibernateTransactionManager) and therefore allows for side-effect free hard read-only behavior... and also avoiding the need for post-transaction cleanup. This brings the same benefit to JpaTransactionManager now.

As for the flushing question, we were specifically wondering whether an explicit query-level FlushMode.MANUAL setting in addition to session-level FlushMode.MANUAL makes any difference... in particular next to Session.setDefaultReadOnly(true). As far as I can see in current Hibernate code, a query-level FlushMode simply temporarily overrides the session-level FlushMode, so there doesn't seem to be any need for repeating MANUAL there.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 5, 2018

MIhalcea Vlad commented

That's great to hear. I'll test it once 5.1 RC1 is released.

As for the query-level flush setting, I don't see how it could make any difference to the Session-level one. If the Session is set to FlushMode.MANUAL, no Query is allowed to flush anyway.

Thanks for integrating it. 

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 15, 2018

Neale Upstone commented

Juergen Hoeller Would you consider this for backport to 4.3.x?

It appears a safe (and sounding significant) performance improvement, which multiplied across many 4.3.x applications would save quite some CPU cycles and energy consumption.  Naturally I'm thinking anything that'll help save the snow in Austrian ski resorts.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 15, 2018

Juergen Hoeller commented

I'd expect all the Austrian-affecting Spring/Hibernate applications to upgrade to 5.1 immediately in any case ;-)

On a serious note, it's actually mostly memory saved here since in a regular Spring read-only transaction no flush is ever being triggered... and even there, it's mostly memory being made available to the GC earlier than before. It might not have much effect in a typical short-lived transaction at all if the GC only kicks in after the transaction in any case.

CPU cycles would only really be saved in case of a user-level flush attempt. Ironically this is exactly the problem with backwards-compatibility: If a user tried such a manual flush() call in a Spring read-only setup, it would currently override the read-only setting and actually flush all loaded entities. As of 5.1, any such manual flush will be in vain, and entities loaded within a read-only transaction are strictly read-only. While this is arguably ok for our intentions, I'm sure that some existing code out there - maybe totally unaware of the read-only flag set somewhere around it - would break, and it'd be totally non-obvious to the innocent patch release upgrader. This in particular applies to JPA access code (since JPA has no read-only or manual flush concept) but also to native Hibernate access code.

All in all, I'm willing to enforce and defend such stricter semantics in a new feature release but I'm not sure that backporting to 4.3.x would be wise here. The JPA part is rather involved to begin with since it needs an extension in the dialect contract and spans a wider range of Hibernate versions in 4.3.x. Even for the native Hibernate part, a backport might cause more hard-to-track harm than noticeable benefit.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 16, 2018

Neale Upstone commented

Thanks Juergen Hoeller.  I'll just have to push the upgrade forwards in our case. I suspected that might be the answer, but it was worth checking.

@mjparme

This comment has been minimized.

Copy link

commented Aug 21, 2019

There is conflicting information in the Spring docs, this issue suggests setting readOnly on a transaction didn't previously propagate down to the Hibernate session; however, the Spring Data doc says setting readOnly = true on a transaction does cause Hibernate to skip dirty checks:

https://docs.spring.io/spring-data/jpa/docs/1.5.0.RELEASE/reference/html/jpa.repositories.html

Excerpt (section 2.5.1):

"Note
It's definitely reasonable to use transactions for read only queries and we can mark them as such by setting the readOnly flag. This will not, however, act as check that you do not trigger a manipulating query (although some databases reject INSERT and UPDATE statements inside a read only transaction). The readOnly flag instead is propagated as hint to the underlying JDBC driver for performance optimizations. Furthermore, Spring will perform some optimizations on the underlying JPA provider. E.g. when used with Hibernate the flush mode is set to NEVER when you configure a transaction as readOnly which causes Hibernate to skip dirty checks (a noticeable improvement on large object trees)."

Which is it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.