Panache#count() leads to additional update statements in Hibernate ORM

[Serkan80]

Describe the bug

When calling the count method on the PanacheEntity, the call is made with dirty checking (~statefull).

This results in unnecessary and unexpected extra SQL calls. And also causes errors, because we have an auditlog which is triggered via a db trigger, because it receives an insert & update record at the same time, which causes not uniqueness error on the primary keys.

See below for an example.

Expected behavior

The count call should be stateless, not resulting in an extra update call.

Expected sql calls should be:

select count ....
insert into ...

Actual behavior

This results in the following queries:

select count ....
insert into ...
update entity set (all the fields) where id = ...

How to Reproduce?

    @Transactional
    public MyEntityDto save(MyEntityDto dto) {
        var entityExists = MyEntity.count("name = ?1", dto.name) > 0;

        if (entityExists) {
            throw new WebApplicationException("Entity(name=%s) already exists".formatted(dto.name), 400);
        }

        var entity = Mapper.mapToEntity(dto);
        entity.persist();
        Log.infof("Entity(name=%s) created", entity.name);

        return Mapper.mapToDto(entity);
    }

Output of uname -a or ver

windows 10

Output of java -version

JDK 17 Temurin

GraalVM version (if different from Java)

No response

Quarkus version or git rev

3.2.6.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Maven 3.8.x

Additional information

A quick fix is by replacing the count with find:

 var entityExists = MyEntity.find("name = ?1", dto.name)
                            .withHint(HINT_READONLY, true)
                            .count() > 0;

It would be nice if the count() method was by default stateless (= with HINT_READONLY = true)

[quarkus-bot]

/cc @FroMage (panache), @loicmathieu (panache)

[FroMage]

@yrodiere and @Sanne on #36009 imply this may be a Hibernate ORM bug. What DB are you using?

[Sanne]

I don't think we have enough information to assume it's an ORM bug. What makes you suspect this?

[yrodiere]

Because of what I said, I think:

First, why would a count query before a persist() call lead to an additional update after the persist() call... ? It looks a lot like a bug in ORM, doesn't it?

Though I do agree we need to have a closer look as there could be weird configuration or mapping at play. I wonder if the reproducer in the description truly is enough to trigger this? There might be getters with side effects at play... ?

[geoand]

@yrodiere what's the status of this?

[yrodiere]

@yrodiere what's the status of this?

Needs investigation on the root cause, from what I can see. It's in the backlog, someone will have a look when they have time.

[mbladel]

FWIW, what I can see from the current implementations of io.quarkus.hibernate.orm.panache.common.runtime.AbstractJpaOperations#count is that a simple SELECT COUNT(*) FROM <entity_name> query is created, and this has no effect to the persistence context: no entity instance is loaded at all, so no "dirty checking" would be executed when flushing the session.

I suspect there might be additional application logic (unrelated to the simple count operation) that changes the state of the persisted entity.

[yrodiere]

FWIW, what I can see from the current implementations of io.quarkus.hibernate.orm.panache.common.runtime.AbstractJpaOperations#count is that a simple SELECT COUNT(*) FROM <entity_name> query is created, and this has no effect to the persistence context: no entity instance is loaded at all, so no "dirty checking" would be executed when flushing the session.

Already done recently: #41620

I suspect there might be additional application logic (unrelated to the simple count operation) that changes the state of the persisted entity.

Right. I suspect that reproducer is not enough, but someone would need to have a look.

[mbladel]

Already done recently: #41620

I'm guessing you're referring to switching to the new Hibernate SelectionQuery#getResultCount() API, introduced with https://hibernate.atlassian.net/browse/HHH-16931. That PR addressed getting counts for existing Panache queries, true, but I was thinking of the count operation on Panache entities themselves.

That is handled in a different place (AbstractJpaOperations IIUC), and I think it would benefit from the new standardized API as well: Hibernate applies some clever tricks on the generated count query to ensure the count is correct and retrieved optimally. I've created #44188 to address this.

[Serkan80]

I think this issue was solved by someone else by adding WITH_HINT to true in another issue. But I can’t find the issue anymore.

Anyways, the reason why the extra update statement was called, might have to do with:

@Transactional
public MyEntityDto save(MyEntityDto dto) {
   …

    return Mapper.mapToDto(entity);
}

This mapper is calling the getters of the entity to create and return a new dto, maybe that was causing it.

But as I stated in my first post, as soon as I replace Panache#count with Panache#find with hints set to true, the update statement doesn’t happen.

[FroMage]

We should check whether the new getResultCount also sets hints to read-only

[mbladel]

@FroMage it doesn't, but as I already said Hibernate count APIs are not selecting any entity instance - thus not adding it to the persistence context, so it's not possible that an update be triggered from it. There must be something persisting the entity and causing it to be dirty in the user's application logic.

[Sanne]

We should check whether the new getResultCount also sets hints to read-only

I don't think we should do that; there are two possibilities regarding what was happening:

• [either] there were previous changes which needed to be flushed to make sure that the count operation was correctly taking them into account (an auto-flush being triggered). You'd not want to disable this.
• [or] there were changes on the entity being triggered by the user code; this could have been unintentional in this case e.g. could have been a wrong hashcode implementation but there are also legit use cases which we shouldn't break.

To me this looks like working as intended, unless we get a better reproducer clarifying details.

Queries have no reason to be flagged explicitly read-only, ORM should do the "optimal thing TM".

[Serkan80]

We should check whether the new getResultCount also sets hints to read-only

I don't think we should do that; there are two possibilities regarding what was happening:

• [either] there were previous changes which needed to be flushed to make sure that the count operation was correctly taking them into account (an auto-flush being triggered). You'd not want to disable this.
• [or] there were changes on the entity being triggered by the user code; this could have been unintentional in this case e.g. could have been a wrong hashcode implementation but there are also legit use cases which we shouldn't break.

To me this looks like working as intended, unless we get a better reproducer clarifying details.

Queries have no reason to be flagged explicitly read-only, ORM should do the "optimal thing TM".

@Sanne How do you explain that the update statement doesn’t occur when you replace Panache#count with Panache#find with readonly set to true ? And I also don’t understand why the count method should not set the readonly hint to true by default.

The reproducer is clear:

After the persist call, a new dto is created which only calls the getters of the entity.

[Sanne]

How do you explain that the update statement doesn’t occur when you replace Panache#count with Panache#find with readonly set to true

In case of a count query, the table space might need to trigger an auto-flush; I'm not sure why that would happen in your specific case, but it might be necessary in some circumstances. In case of a find operation, such mechanism wouldn't be necessary to be triggered.

And I also don’t understand why the count method should not set the readonly hint to true by default.

It shouldn't be necessary to make any difference in performance / efficiency. However forcefully disabling this w/o it being an explicit opt-in by the developer could cause problematic side effects, that's why I'm suggesting to not do that.

The reproducer is clear

Fair enough, I didn't try the reproducer and I might be missing something but your other comments above seem to suggest it's not reproducing anymore? "I think this issue was solved by someone else ..." Also, several very capable colleagues looked already - could you clarify where we are and if there is a reproducer actually showing a problem?

N.B. I'm not claiming that it's impossible for there to be an issue in Hibernate ORM, I'm just suggesting that if there's a bug in ORM we should fix it properly, while adding a hint in the Panache layer wouldn't be the best approach. We don't want to try to hide one bug by adding another bug in a different layer ;) But unless there's a working reproducer, we should close this.

[yrodiere]

Hey @Serkan80 ,

The reproducer is clear:

After the persist call, a new dto is created which only calls the getters of the entity.

I'm afraid the reproducer is incomplete, e.g. there's no mapping for the entities involved, so there's still work involved in creating an actual reproducer that someone can run to debug the problem. I also suspect that writing that reproducer will involve some other code, because the behavior you're witsnessing is very, very unpexected -- maybe I'm wrong.

For now we'll be focusing on other issues that do have a working reproducer. If you have time to add one here, that would be great. Thank you.

EDIT: To be clear, by "reproducer" we generally mean "a GitHub repository or ZIP with a full, but minimal, Quarkus application demonstrating the issue"

[Sanne]

Agreeing with @yrodiere and since I said approximately the same 2 weeks ago, I'll go ahead and close this.

Feel free to reopen when a proper reproducer can show me wrong! ;)