feat: read only sessions #10077

loicmathieu · 2020-06-17T15:28:20Z

This is a new attemp to provides so called read only transactions.

Instead of using a CDI interceptor like with #7455 it put inside the transaction synchronization registry the transaction configuration object, retrieve it inside Hibernate and update the session.

I made some choices that should be discussed:

I put the TransactionConfiguration annotation inside the registry for future usage if needed. Maybe I need to create a separate object.
I didn't set back the session to it's previous state at the end of the transaction but as the session is transaction bounds it should be safe right ?

If we agree it is a better implementation as #7455 I'll provides more test and some documentation

loicmathieu · 2020-06-17T15:29:20Z

/cc @Sanne @stuartwdouglas @famod a less complex way of providing readonly transactions by using the transaction synchronization registry.

famod · 2020-06-17T21:40:40Z

I'd perfer this PR, nice work!

I'd agree that resetting readOnly and FlushMode seems unnecessary.

famod · 2020-06-17T21:42:07Z

...hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/ReadOnlyTransactionTest.java

+        assertTrue(session.isDefaultReadOnly());
+        assertEquals(FlushMode.MANUAL, session.getHibernateFlushMode());
+
+        assertThrows(Exception.class, () -> forceRO());


Exception is a bit "broad". Any chance to narrow it down to a more specific exception?

Yes, tests need to be reworked

famod · 2020-06-17T21:46:16Z

...hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/ReadOnlyTransactionTest.java

+        Address adr = new Address();
+        adr.setStreet("rue du paradis");
+        entityManager.persist(adr);
+        entityManager.flush();


When called from testRO(), does this flush() make a difference?

Today a colleague of mine mentioned that in Spring Boot with read-only transactions you don't realize when you accidentally try to persist/update something.
Now I am wondering whether this would also happen in Quarkus when flush() is not called (calling flush() is not necessarily the default case).

I will add more tests ...

stuartwdouglas · 2020-06-17T21:55:03Z

.../src/main/java/io/quarkus/narayana/jta/runtime/interceptor/TransactionalInterceptorBase.java

@@ -119,6 +125,8 @@ protected Object invokeInOurTx(InvocationContext ic, TransactionManager tm, Runn
                tm.setTransactionTimeout(currentTmTimeout);
            }
        }
+        // put the transaction configuration inside the synchronization registry to access it from Hibernate
+        transactionSynchronizationRegistry.putResource(CONFIG_KEY, configAnnotation);


I think that we also need to make sure that the transaction is rolled back rather than committed if it is read only.

I'm not sure about this one. Read only transactions are only a hint sent to the underlying data-remated technolohy (here, the Hibernate Session), they are not realy a property of the transaction. If you use transaction with JDBC (or MongoDB), they will have no effect (this needs to be documented as a hint not honored by all data access layer).

Of course, we can set the transaction to be rollback only at the transaction manager level. But read only transaction are not a JTA standard so I will be reluctant that they act at JTA level.

Two things to keep in mind:

some databases can actually start a "read only transaction", and this will get you performance benefits; but to take advantage of this, you need to be able to start the transaction in RO mode from the very beginning. Might be out of scope? e.g. I don't think Agroal allows this today.

some (other) databases actually perform better on commit than on rollback. So I'm not sure about rolling back being a good idea as an enforced mode - I'm aware it's popular among some people, but I'd not be sure about enforcing this.

Why would you roll it back? Just to be really sure that no changes are written? To achieve such an effect, I think it would be more useful to actually wrap our data-access APIs with a read-only decoration, so to throw a clear exception on any attempt.

If we wanted to do that, I suppose we'd need the TX to be marked as transaction since its creation (so to affect any resources we provide within its scope); which would have the added benefit of allowing optimisation #1 above, should we want to implement that in the future.

I just don't like the idea that a read only TX can still commit data, and if this does only affect hibernate it just feels wrong that you could still use JDBC to do writes in a supposed read only TX.

I agree with that. Would be nice to be able to wrap such other APIs to prevent it.

So we have two options I believe we all could agree on:

convert this into a @SessionConfiguration so that it's clearer that this is a tuning hint for Hibernate only.

Implement a real read-only transaction support.

While 2. is probably more valuable, it's clearly a long way to get there: at very least it needs patches in multiple components.

So let's go with approach 1 and make it very clear that:

this is Hibernate specific

it's only a hint

Hopefully such annotation would be used for several more useful tuning options? It begs the question of how a single annotation becomes extensible, maybe it will need multiple annotations?

OK for converting to @SessionConfiguration.
There is two ways of implementing this:

Use a @Transactional CDI interceptor that then try to locate this @SessionConfiguration annotation: the session configuration will have the same scope as the transaction, and we can still use the Transaction Synchronization Registry to communicate between the interceptor and the entity manager factory.

Use a @SessionConfiguration CDI interceptor: in this case we cannot be sure that we are in the context of a transaction so we cannot use the Transaction Synchronization Registry as a communication layer between the interceptor and the entity manager factory. We can still use Arc inside the interceptor to access the entity manager. If we implement this, a user could have a read only scope smaller than the transaction one (and possibly switch to read only / read write multiple times inside the same transaction).

WDYT ?

@Sanne can you give your advice on how to implement read only sessions with @SessionConfiguration as we discuss in this thread ?
I proposed two way to do this and need your input on which one is better ;)

@Sanne can you give your advise on the best way to update this old PR to use @SessionConfiguration as we discussed ? I proposed two ways here #10077 (comment) can you give me some feedback ?

I would go with a take on approach 1) (some of this may be slightly overkill)

Add an @TransactionConfig meta annotation

@SessionConfiguration is annotated with @TransactionConfig

The transaction interceptors are modified to discover @TransactionConfig annotations on the method or class, and put them into the TSR

Have hibernate pull this info out of the TSR and use it to decide on how to proceed

There will be caching needed here, we don't want to be reading annotations every transaction. I am not sure if @TransactionConfig is really needed, but the alternatives are hard coding a hibernate annotation into the TX subsystem, or sticking every annotation into the TSR (which may end up having perf impact).

@Sanne does this sound reasonable?

stuartwdouglas · 2020-06-18T21:59:54Z

So it seems like all this is really just to run 3 lins of code:

            Session session = newEm.unwrap(Session.class);
            session.setDefaultReadOnly(true);
            session.setHibernateFlushMode(FlushMode.MANUAL);

Why don't we just put these in a utility method? I don't really think the annotation based approach buys you much here.

loicmathieu · 2020-06-19T08:07:30Z

Why don't we just put these in a utility method? I don't really think the annotation based approach buys you much here.

Developers used to use annotation for such contextual configuration. Moreover, in Panache, they don't manipulate the entityManager by themself. I also think it should be done as early as possible, ideally at session creation time (like it is done today).

gastaldi · 2020-10-13T14:38:10Z

Are we yay or nay on this one? @stuartwdouglas @Sanne ?

stuartwdouglas · 2020-10-13T22:19:16Z

I think it should be done as a SessionConfiguration rather than a TransactionConfiguration, to make it clear what is going on. There are also other uses for SessionConfiguration, e.g. you could use it to configure flush mode or ask for a stateless EM.

loicmathieu · 2020-10-15T09:38:49Z

@stuartwdouglas OK for SessionConfiguration but as I explained here #10077 (comment) it could be done in two ways and I need guidance on which one seems to be the best one.

loicmathieu · 2020-11-30T16:08:52Z

@stuartwdouglas @Sanne I update the impl as suggested.

stuartwdouglas

Mostly looks really good.

stuartwdouglas · 2020-11-30T22:58:34Z

...main/java/io/quarkus/hibernate/orm/runtime/entitymanager/TransactionScopedEntityManager.java

+                Session session = newEntityManager.unwrap(Session.class);
+                session.setDefaultReadOnly(true);
+                session.setHibernateFlushMode(FlushMode.MANUAL);
+                //TODO maybe set back the flush mode / default read only somewhere ?


I don't think we need this todo, current impl is correct IMHO

stuartwdouglas · 2020-11-30T23:03:40Z

.../src/main/java/io/quarkus/narayana/jta/runtime/interceptor/TransactionalInterceptorBase.java

@@ -96,6 +105,35 @@ private TransactionConfiguration getTransactionConfiguration(InvocationContext i
        return configuration;
    }

+    private Map<Class<?>, Annotation> getAdditionalTransactionalConfiguration(InvocationContext ic) {


I think this should be cached, as it is slow. Ideally we would pre-compute this at build time.

Also does this work in native mode? I think it will but an integration test would be helpful.

gavinking · 2020-12-01T09:32:22Z

I think it's a mistake to add a new annotation for this, especially when the annotation has just one boolean member.

I could live with it if:

it were an API that simplified the "configuration" of various interesting properties of the session (filters, default cache and lock modes, etc), or if
it were a much more general-purpose @ReadOnly construct, or perhaps even if
it were a just an additional attribute of some pre-existing annotation.

But in the current form it looks like a wart. It looks very much under-designed, like it's trying to solve one very very specific problem without stepping back and contextualizing the problem in terms of a class of similar problems.

gavinking · 2020-12-01T09:40:57Z

And I even have an additional objection: I don't think we should be defining APIs and new programming models in io.quarkus.hibernate.runtime.

loicmathieu · 2020-12-01T11:15:18Z

@gavinking thanks for your feedback, the current design has been discussed with @Sanne and @stuartwdouglas in this PR (and another one) comments.

First, I propose to add a readOnly attribute to @TransactionConfiguration but as there is no such thing as readonly transaction, and as this transaciton is on Narayana that can be used without Hibernate, it has been decided to reside on its own annotation.

This new annotation has been called @SessionConfiguration so we can add more configuration option later, I can add more of these now if you want.

As you detected, it has been put on the wrong package, it should reside inside io.quarkus.hibernate.orm package. I'll change it.

gavinking · 2020-12-01T11:20:00Z

Right, but the thing is that I don't see how other kinds of session configuration fit into this model. Specifically I don't see how filters fit in. Annotations simply can't do some things that calling functions can do.

That's what I mean when I say it looks underdesigned. I don't think it addresses (even in principle) all the things one wants to do when "configuring" a session.

gavinking · 2020-12-01T11:22:28Z

FTR I agreed with Stuart's original comment months ago that this would be better as an API and I didn't speak up at the time because I had nothing additional to add to that. Apparently there have been additional discussions since then which were not captured here on the issue. (They should have been, IMO.)

loicmathieu · 2020-12-10T17:48:36Z

@gavinking some discussions has been made on the #7455 that has been closed on the favor of this one.

An API to configure the session already available as one can inject the EntityManager (or get it from a Panache entity/repository) and extract the Session from it. Something like this (not tested)

@Inject EntityManager entityManager;

Session session = entityManager.unwrap(Session.class);
session.setDefaultReadOnly(true);
session.setHibernateFlushMode(FlushMode.MANUAL);

We can of course provides something inside Panache to facilitate.

The choice of an annotation is that it's a very common task to set the session as readonly for performance reason, and some user ask for it. Spring provides the same facility.

Now, if you think that it's a badly design API, I'm open for suggestion.

Maybe @stuartwdouglas or @Sanne have some throught on it ?

gavinking · 2020-12-10T17:57:19Z

Look, the over-arching problem we have here is that everyone is trying to add their own little bits and pieces to Quarkus to try and make Hibernate easier to use. But it doesn't. It makes it harder to use because you're smearing the API of Hibernate out across many different modules and the user has no idea where to go to get the thing they're looking for.

Hibernate-related APIs belong in hibernate-xxx.jar, not in Quarkus. The Quarkus extensions exist to provide integration code, not APIs.

We really need to get a handle on this before it gets out of control. If Hibernate is missing some API that Quarkus users are going to need, go to https://github.com/hibernate and add it there. Because otherwise we're going to end up with a clusterfuck of everyone adding their own teensy little Hibernate APIs that address some teensy discomfort but without a global view of how to solve problems without all sorts of inconsistency and redundancy.

loicmathieu · 2020-12-11T15:31:58Z

@gavinking I understand your point, and yes, the Hibernate ORM extension is for integration purpose not API.

@Sanne and @gsmet are the maintainers of the Hibernate ORM Quarkus extension, I'll let them decide if we close this one and the corresponding #6414 issue as won't fix, or provide something else somewhere else (on Panache which have an API, or on Hibernate itself as you suggested).

gsmet · 2021-04-01T09:00:48Z

Let's close this one for now. We can rediscuss it later.

boring-cyborg bot added area/hibernate-orm Hibernate ORM area/narayana Transactions / Narayana labels Jun 17, 2020

famod reviewed Jun 17, 2020

View reviewed changes

stuartwdouglas requested changes Jun 17, 2020

View reviewed changes

loicmathieu mentioned this pull request Oct 16, 2020

feat: read only transactions #7455

Closed

loicmathieu force-pushed the feat/read-ony-tx-2 branch from e5717aa to b09933f Compare October 20, 2020 12:18

fix: avoid CDI warning in ObjectMapper

9711989

loicmathieu force-pushed the feat/read-ony-tx-2 branch from b09933f to 0afb1a9 Compare November 25, 2020 10:38

ghost added the area/persistence OBSOLETE, DO NOT USE label Nov 25, 2020

loicmathieu changed the title ~~feat: read only transactions (reloaded)~~ feat: read only sessions Nov 25, 2020

loicmathieu marked this pull request as ready for review November 25, 2020 10:39

loicmathieu requested review from Sanne and stuartwdouglas November 25, 2020 10:39

loicmathieu force-pushed the feat/read-ony-tx-2 branch from 0afb1a9 to a92bfa5 Compare November 30, 2020 16:01

stuartwdouglas requested changes Nov 30, 2020

View reviewed changes

Provides read-only session

657aedc

loicmathieu force-pushed the feat/read-ony-tx-2 branch from a92bfa5 to 657aedc Compare December 1, 2020 11:16

Base automatically changed from master to main March 12, 2021 15:54

gsmet closed this Apr 1, 2021

quarkus-bot bot added the triage/invalid This doesn't seem right label Apr 1, 2021

loicmathieu mentioned this pull request Jun 8, 2022

Provide read-only transactions. #6414

Open

escay mentioned this pull request Dec 27, 2022

stateless entity manager jakartaee/persistence#374

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: read only sessions #10077

feat: read only sessions #10077

loicmathieu commented Jun 17, 2020

loicmathieu commented Jun 17, 2020

famod commented Jun 17, 2020

famod Jun 17, 2020

loicmathieu Jun 18, 2020

famod Jun 17, 2020 •

edited

Loading

loicmathieu Jun 18, 2020

stuartwdouglas Jun 17, 2020

loicmathieu Jun 18, 2020

Sanne Jun 18, 2020

stuartwdouglas Jun 18, 2020

Sanne Jun 18, 2020

Sanne Jun 18, 2020

loicmathieu Jun 19, 2020

loicmathieu Sep 11, 2020

loicmathieu Oct 13, 2020

stuartwdouglas Oct 15, 2020

stuartwdouglas commented Jun 18, 2020

loicmathieu commented Jun 19, 2020

gastaldi commented Oct 13, 2020

stuartwdouglas commented Oct 13, 2020

loicmathieu commented Oct 15, 2020

loicmathieu commented Nov 30, 2020

stuartwdouglas left a comment

stuartwdouglas Nov 30, 2020

stuartwdouglas Nov 30, 2020

gavinking commented Dec 1, 2020

gavinking commented Dec 1, 2020

loicmathieu commented Dec 1, 2020

gavinking commented Dec 1, 2020

gavinking commented Dec 1, 2020

loicmathieu commented Dec 10, 2020

gavinking commented Dec 10, 2020 •

edited

Loading

loicmathieu commented Dec 11, 2020

gsmet commented Apr 1, 2021

feat: read only sessions #10077

feat: read only sessions #10077

Conversation

loicmathieu commented Jun 17, 2020

loicmathieu commented Jun 17, 2020

famod commented Jun 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

famod Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartwdouglas commented Jun 18, 2020

loicmathieu commented Jun 19, 2020

gastaldi commented Oct 13, 2020

stuartwdouglas commented Oct 13, 2020

loicmathieu commented Oct 15, 2020

loicmathieu commented Nov 30, 2020

stuartwdouglas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavinking commented Dec 1, 2020

gavinking commented Dec 1, 2020

loicmathieu commented Dec 1, 2020

gavinking commented Dec 1, 2020

gavinking commented Dec 1, 2020

loicmathieu commented Dec 10, 2020

gavinking commented Dec 10, 2020 • edited Loading

loicmathieu commented Dec 11, 2020

gsmet commented Apr 1, 2021

famod Jun 17, 2020 •

edited

Loading

gavinking commented Dec 10, 2020 •

edited

Loading