feat: read only transactions

[loicmathieu]

Fixes #6414

@FroMage @Sanne this is a prototype of read only transaction support.
It uses a CDI interceptor inside the Hibernate extension to set some optimization at the session level.

Please advise if it is the right way to do this.

I don't know if adding a CDI interceptor is OK, I don't know the tradeoff (preformance cost ?)

[Sanne]

Thanks @loicmathieu !

Bear with me, my CDI fu isn't very strong - but I have the impression this would allow to use the same EntityManager instance before (and after) the read-only transaction?

I don't think we should allow that, it gets very tricky to handle correctly - let me elaborate:

This implies that objects can be attached to the EM during such read-only block, and be flagged to be treated as "read only", but after the block is ended (or before) people are allowed to add read-write objects - so you end up having a mixture of read-only and read-write managed objects within the same sesssion, this could get ugly and unpredictable.

A practical example, is this failure to write changes:

• enter the RO transaction
• object A is loaded within the read-only block, so it's marked ro.
• exit the RO transaction
• begin a RW transaction
• some object A is loaded (either by bad luck or intentionally - hard to control!)
• changes are applied to A
• RW transaction is committed -> state is discarded, as A still is in ro state.

You could have similar things happening, the opposite way:

• begin a RW transaction
• object A is loaded, marked as read-write
• a "RO transaction block" is entered either after wrapping up the RW transaction, or not
• some changes are applied on A - expecting them to be volatile as we're in a RO scope
• ..
• the volatile changes are eventually committed to the database

It gets more tricky and more complicated when you consider object graphs, the fact that many of these managed entities will be relating to each other - and some will be loaded as side-effect so possibly ending up in the wrong "scope".

In summary, let's make sure when entering the RO scope we're not reusing an existing RW EntityManager, and that we close it on exiting of such state. Of course it would be nice to actually share the same RO EntityManager across multiple RO blocks, so to leverage the automatic caching layer.

[loicmathieu]

@Sanne I check what Spring does and it set the session to readOnly at transaction begin and set back the session to the old state at the end of the transaction.

The current implementation is not tied to a transaction because I didn't add the@Transactional annotation to the interceptor binding.

But even with the addition of the annotation, there can still be mixed RO/RW transactions as you describe because the transaction support is inside Narayana that is not aware of Hibernate. As transactions can be nested in Narayana we should ideally merge the two interceptors and set the session to readOnly inside io.quarkus.narayana.jta.runtime.interceptor.TransactionalInterceptorBase.invokeInOurTx.

I updated my current implementation to mandate a transaction (by checking that the target have the @Transactional annotation inside my interceptor) and to allow setting the readOnly attribute only at top level transaction (this is done inside TransactionalInterceptorBase.checkConfiguration() as we did for transaction timeout) so we could no longuer mix RW and RO transaction. This should do the trick and avoid merging both interceptors.

If I understand correctly the code inside TransactionScopedEntityManager in case of transaction the EntityManager is scoped to a transaction (and so is a session right?): it is open when a transaction begin and close after it's completion, so the new implementation should be safe.

If this is not enough to implements correctly read only transactions, we will need a deeper collaboration between Narayana and Hibernate and this will be out of my knowledge range :)

[Sanne]

thanks @loicmathieu , setting the top level transaction to RO seems very sensible to me.

EntityManager is scoped to a transaction (and so is a session right?)

yes, EntityManager and Session are 1:1. They are actually the same instance: one class implements both interfaces to save on allocations (we used to have a wrapper but it's gone now).

But even with the addition of the annotation, there can still be mixed RO/RW transactions as you describe because the transaction support is inside Narayana that is not aware of Hibernate.

I'm not sure I understand this. You mean people could start a nested R/W transaction within a RO parent transaction?

We can definitely ask the Narayana team to give us some extra integration magic. /cc @ochaloup , you might want to be aware of this feature and maybe check we're not planning anything dumb? :)

[loicmathieu]

@Sanne

I'm not sure I understand this. You mean people could start a nested R/W transaction within a RO parent transaction?

Normally not thanks to the trick I implemented to check that the @TransactionConfiguration is only set at the transaction entry method.

And yes, if someone from Narayana can have a look and validate the implementation or propose a better one this could be good :)

[ochaloup]

hi @loicmathieu and @Sanne, sorry for a bit delayed answering time.

I checked the Spring issue where they provide the similar change spring-projects/spring-framework#21494 and from that perspective the way and the reasoning sounds good.

First, let me recap of how I understand the change. The user now gets a way how to declare the method to work in "read-only" mode. This affects only the JPA access (no changes when the JDBC datasource is used). This makes the user responsible to determine the read-only method beforehand and answers for cases when the code (e.g. a third party one which is invoked at the scope of the started transaction) makes some writing operations eventually. Such a failure is then the responsibility of the user.

If I understand right the way how the EntityManager is gained in Quarkus it's by a Singleton producer (https://github.com/quarkusio/quarkus/blob/master/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/DefaultEntityManagerProducer.java). That means any EntityManager used refers to the same instance. Does not mean that then unwrapping the session makes the read-only change being applied not only to the transaction related instance? (I haven't checked that by code I'm just trying to understand what's happening.)

Regarding the implementation. As the interceptor makes changes in the JPA related behaviour it seems to me being more logical to be part of the hibernate/orm codebase than the transaction interceptors directly (btw. would not cause having this handling being part of the TransactionInterceptorBase a circular dependency issue?) But there could be other reasons to add that to transactional interceptor directly which I could not see now. Maybe e.g. if we want to enhance this to apply the improvement for the agroal JDBC datasources as well and the handling should be maintained from one place.

A minor questions

• why the implementation uses the checkTransactional and does not define the @Transactional as the interceptor biding annotation at definition of the class?
• if the interceptor is part of the orm/hibernate I think it should not use the logger directly from Narayana jtaLogger.i18NLogger.
• I think few more tests with the real DAOs should be provided.

One major doubt is about what happens on the following case:
client -> call to method n.1 annotated -> @Transactional(value = TxType.REQUIRES_NEW) @TransactionConfiguration(readOnly = true) -> call to method n.2 annotated -> @Transactional(value = TxType.REQUIRES_NEW) @TransactionConfiguration(readOnly = false)
On the second call (to the method n.2) the new transaction is created which is not expected to be part of the read only management. But I assume the current interceptor won't make the second method being managed as read-write, or will it?

[loicmathieu]

@ochaloup

If I understand right the way how the EntityManager is gained in Quarkus it's by a Singleton producer

In cas of transactions, we have one TransactionScopedEntityManager per transaction. So if we restrict the usage of the read only transaction to @Transactional annotated method we should be safe.

why the implementation uses the checkTransactional and does not define the @transactional as the interceptor biding annotation at definition of the class?

I try this, but interceptor bindings are OR and not AND, so if I add two annotations, the interceptor is called when one of the two is used and I want it to be called when the two are used.

if the interceptor is part of the orm/hibernate I think it should not use the logger directly from Narayana jtaLogger.i18NLogger.

Will have a look.

I think few more tests with the real DAOs should be provided.

Yes, this is why it's still a draft PR :)

One major doubt is about what happens on the following case:
client -> call to method n.1 annotated -> @transactional(value = TxType.REQUIRES_NEW) @TransactionConfiguration(readOnly = true) -> call to method n.2 annotated -> @transactional(value = TxType.REQUIRES_NEW) @TransactionConfiguration(readOnly = false)

This was also the concern of @Sanne . I definitly must add test about this, I don't know if a new entity manager will be created or not in this scenario. The read-only scope should be the one of the transaction, so starting a new transaction should means defining a new readOnly scope.

Thanks for your feedback.
I'll do more test and gives feedback later on this.

[ochaloup]

@loicmathieu good we can then discuss the progress.

I wonder about one think. You say

I try this, but interceptor bindings are OR and not AND, so if I add two annotations, the interceptor is called when one of the two is used and I want it to be called when the two are used.

It seems to be pretty strange behaviour. I created a small reproducer and it seems it behaves as you describe. I need to check this with someone knowledgeable.

[ochaloup]

Just for info, that strange behaviour of the interceptor binding is probably an issue. It may be followed at #7931

[loicmathieu]

@ochaloup now that #7931 is merged I update the code to use both interceptor binding annotations.

I also add a test with nested @Transactional(value = TxType.REQUIRES_NEW) and discovers that the interceptor didn't works for @Transactional(value = TxType.REQUIRES_NEW) at all.

I will need some help to understand why in case of REQUIRES_NEW when I update the session inside the entityManager inside my interceptor then access the session inside my test, the session has been reset (at least, the read only mode).

[ochaloup]

@loicmathieu you mean the TransactionalInterceptorRequiresNew is invoked but yours ReadOnlyTransactionInterceptor is not invoked at all? It almost sounds like some other Arc CDI issue (but I'm checking just briefly).

[loicmathieu]

@ochaloup no, both interceptors are correctly called.
But something should happens at hibernate level ...

[emmanuelbernard]

@loicmathieu I'm checking the PR (albeit late), I see no doc and that makes me cry on the inside. Do you think we could have a paragraph explaining when and how to use such feature. Otherwise it iwll be limited to the 5 folks that followed that issue ;)

[loicmathieu]

@emmanuelbernard don't cry! This is a draft PR that why there is no doc.
We must document it and explain what it does and it's impact on performance and functionalities.

For the moment, it is not working properly, as soon as I find a way to implement it properly I will add documentation to it and make it ready for review.

[famod]

@loicmathieu I guess you still have to find a way to implement it properly? Thanks.

[loicmathieu]

@famod yes, I struggeled with the case of @Transactional(value = TxType.REQUIRES_NEW), in this case the readonly interceptor have a different Hibernate session than the one used by the intercepted method, so readonly has no effect.
I'm not a transaction expert so I don't know what happened and how to fix it ...

[famod]

@loicmathieu I debugged the failing test ReadOnlyTransactionTest.testSubTransactions() and it seems ReadOnlyTransactionInterceptor isn't called at all for that test.
That changes if I extend @Transactional at the interceptor to @Transactional(Transactional.TxType.REQUIRES_NEW). The interceptor then kicks in but it does also do that for that "inner" newTransaction() call, although that method is annotated with @TransactionConfiguration(readOnly = false).

So it seems there is an interceptor binding issue.
I am not really an expert in this but maybe you cannot say that you want to have all bindings met? Currently there are two annotations at this interceptor that are in turn annotated with @InterceptorBinding: @Transactional and @TransactionConfiguration...

[famod]

Looking at https://github.com/quarkusio/quarkus/blob/master/extensions/narayana-jta/runtime/src/main/java/io/quarkus/narayana/jta/runtime/interceptor/TransactionalInterceptorBase.java and all those TxType specific subclasses in the same package, it really seems you need one concrete interceptor per TxType.
Transactional.rollbackOn and .dontRollbackOn are not relevant because they are annotated with @Nonbinding.

But instead of binding to @Transactional, ReadOnlyTransactionInterceptor could maybe instead just bind to @TransactionConfiguration(readOnly = true) and could then use something like in TransactionalInterceptorBase.getTransactional(InvocationContext ic):

        Set<Annotation> bindings = InterceptorBindings.getInterceptorBindings(ic);
        for (Annotation i : bindings) {
            if (i.annotationType() == Transactional.class) {
                return (Transactional) i;
            }
        }

Btw, since you added @InterceptorBinding to TransactionConfiguration you'll probably need to add @Nonbinding to .timeout.

[famod]

That's strange: For testing purposes I removed @Transactional entirely from the interceptor (leaving only @TransactionConfiguration(readOnly = true)), but the interceptor is still invoked for that "inner" newTransaction().

[famod]

It seems something goes wrong on this line:

quarkus/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/InterceptorResolver.java

Line 90 in 61d80b8

&& !value.equals(interceptorBinding.valueWithDefault(beanDeployment.getIndex(), annotationField))) {

value ist basically readOnly = true and the result of interceptorBinding.valueWithDefault(...) is readOnly = false, which matches my expectations at this point.

But then in org.jboss.jandex.AnnotationValue.equals(Object) the following happens:

return name.equals(that.name);

So it is basically only comparing readOnly to readOnly and ignores the values.
This seems wrong and explains why the interceptor is running in cases where it shouldn't!

cc @mkouba @stuartwdouglas WDYT?

Btw, for this debugging session I rebased this branch onto master.

[stuartwdouglas]

I don't think you can call it a read only transaction when it is only the EntityManager that is read only, any other transactional resources will work as normal. To me a read only TX implies that the interceptor will roll back the transaction instead of attempting to commit.

IMHO this should be implemented in the TX interceptors, but then exposed as an attribute in the TSR. Hibernate should then be able to determine that the TX is read only when it creates the EM, and set it to read only that way.

[mkouba]

But then in org.jboss.jandex.AnnotationValue.equals(Object) the following happens

@famod Hm, each AnnotationValue subtype should override equals()/hashCode() depending on the type, but in this particular case the org.jboss.jandex.AnnotationValue.BooleanValue does not override these methods. That's a bug in jandex: smallrye/jandex#81

[loicmathieu]

IMHO this should be implemented in the TX interceptors, but then exposed as an attribute in the TSR. Hibernate should then be able to determine that the TX is read only when it creates the EM, and set it to read only that way.

@stuartwdouglas I was also thinking of something like this: putting the transaction configuration inside the TSR and using it at EM creation time. Maybe it'll be a better implementation.
We discuss with Sanne what we called "read only transaction" and we agree it's not exactly a read only transaction but a performance hint that we give to the underlying transacted ressource for performance reason. We call it "read only tx" because it's the way people used to call this ;)

[loicmathieu]

@mkouba you desribe the issue to be only for boolean values right? As a workardoun to test my prototype I can switch to String values and it should works as expected ?

[loicmathieu]

@famod the design with multiple bindings is what we want to do, only trigger this intereptor if the @Transactional annotation is used in conjonction with a @TransactionConfiguration(readOnly=true) annotation. This has been discussed with a transaction expert so this should be OK.

As you discover some interceptor bindig implementation issue, let's wait for it to be fixed to check if the proposed implementation is correct or not.

[famod]

@loicmathieu

the design with multiple bindings is what we want to do

Ok, then you'll probably have to implement at least two interceptors (REQUIRED & REQUIRES_NEW).

FWIW, one alternative to that could be to have the existing tx-interceptors add something to invocationContext.getContextData() that could be checked in the readonly interceptor.

[loicmathieu]

Closing it in favor of #10077