Manually created sequence in postgresql is not working correctly

[svinther]

Describe the bug
Given an Entity with a Long type @Id column, when manually creating the sequence for the id column CREATE SEQUENCE giftSeq the behaviour of assigning is broken.

Expected behavior
id's assignment follow the natural series development 1,2,3,...

Actual behavior
The id's assigned to follow the path 1,2,-46,-45,... until a collision happens

To Reproduce

docker run --ulimit memlock=-1:-1 -d --rm=true --memory-swappiness=0
--name postgres-quarkus-hibernate -e POSTGRES_USER=hibernate
-e POSTGRES_PASSWORD=hibernate
-p 5432:5432 postgres:10.5

docker exec -ti postgres-quarkus-hibernate psql -U hibernate -c "CREATE DATABASE hibernate_db"

docker exec -ti postgres-quarkus-hibernate psql -U hibernate -c "CREATE TABLE Gift(id bigint primary key, name text); CREATE sequence giftSeq;" hibernate_db

Reproducer example here, with test case https://github.com/svinther/quarkus-problem-messysequence

Configuration

%test.quarkus.hibernate-orm.log.sql=true
%dev.quarkus.hibernate-orm.log.sql=true
%dev.quarkus.hibernate-orm.log.bind-param=true

quarkus.hibernate-orm.database.generation=none

# datasource configuration
quarkus.datasource.db-kind=postgresql
quarkus.datasource.username=hibernate
quarkus.datasource.password=hibernate
quarkus.datasource.jdbc.url=jdbc:postgresql://localhost:5432/hibernate_db

Environment (please complete the following information):

• Openjdk 11
• Quarkus 1.7.0.Final

Additional context
If creating the sequence as "CREATE sequence giftSeq start 1 increment 50;" as seems to be hibernates default behavior, then everything seems to work correctly

[mkouba]

I'm no JPA expert but shouldn't be @GeneratedValue#generator() used together with @SequenceGenerator or @TableGenerator?

[svinther]

I'm no JPA expert but shouldn't be @GeneratedValue#generator() used together with @SequenceGenerator or @TableGenerator?

I think you are right. I followed this quickstart guide: https://quarkus.io/guides/hibernate-orm without thinking too much, I guess the guide should be corrected ?

[mkouba]

Yes, would you care to send a PR?

CC @Sanne

[Sanne]

right, this is a bit confusing.

The default ID generation strategy uses an in-memory pool, and assumes an initial value of 1 and a sequence increment of 50 - as these are the values hardcoded in the JPA annotation SequenceGenerator ; when mapping the entities like you did without an explicit sequence definition, it will use the defaults and expect (require!) the Sequence to be compatible with this configuration.

I wonder if we should trigger a warning when the SequenceGenerator isn't explicitly defined, but I'm afraid that would be noisy as we'd not be able to verify if the sequence definition was actually intentionally matching, which would be correct.

[Sanne]

BTW, the quickstart guide isn't necessarily wrong; I'm not sure which entities that example is supposed to use, but it is possible that they have a compatible mapping.

[mkouba]

@Sanne @svinther there is only one example entity in the guide and the fact that there is no @SequenceGenerator is really confusing. Even the entity in the quickstart declares the @SequenceGenerator: https://github.com/quarkusio/quarkus-quickstarts/blob/master/hibernate-orm-quickstart/src/main/java/org/acme/hibernate/orm/Fruit.java#L20-L23

[Sanne]

the fact that there is no @SequenceGenerator is really confusing.

The annotation is not mandatory, so why is that confusing?

The Fruit entity in the quickstart doesn't technically need it either, but I prefer having it as it avoids the need to customize the import script: you'll need either one or the other, or just let Hibernate ORM generate the schema and it will do the right thing.

I'm playing devil's advocate.. I'm open to suggestions but I'm just not sure what to do here. Clearly, the mapped objects and the physical schema need to match, or the ORM won't work - but I hope this doesn't come as a surprise, we can only automate this to some extent.

[mkouba]

The annotation is not mandatory, so why is that confusing?

I'm sorry for my ignorance but if the annotation is not present then we expect a generator of name giftSeq to be defined somewhere because @GeneratedValue.generator() does not correspond to a sequence name, or? At least the javadoc does not mention this.

My point is that if you follow the guide you'll not end up with a functional app...

Clearly, the mapped objects and the physical schema need to match, or the ORM won't work...

quarkus.hibernate-orm.database.generation=drop-and-create should ensure this for the purpose of the guide, or?

[Sanne]

no if it refers to a generator giftSeq then it implicitly expects a Sequence to exist having that name.
More commonly, ORM will generate this sequence during schema creation, or validate that it does indeed exist during schema validation.

[mkouba]

no if it refers to a generator giftSeq then it implicitly expects a Sequence to exist having that name.

Ok, thanks for clarification. In that case, this issue is very likely a bug...

[svinther]

Yes, would you care to send a PR?

CC @Sanne

@mkouba Here goes: #11728

[gavinking]

@Sanne

The default ID generation strategy uses an in-memory pool, and assumes an initial value of 1 and a sequence increment of 50 - as these are the values hardcoded in the JPA annotation SequenceGenerator

Are you sure about that Sanne? Isn't that only the case when you have actually explicitly defined a @SequenceGenerator without specifying allocationSize.

TableGenerator.DEFAULT_INCREMENT_SIZE and SequenceStyleGenerator.DEFAULT_INCREMENT_SIZE are both defined to 1, and isn't that what you get when you don't specify @SequenceGenerator anywhere?

[gavinking]

Yeah, if you just use @Id @GeneratedValue(strategy = SEQUENCE) you get initialValue=1, allocationSize=1. That's what I thought. But perhaps I misunderstood what you were trying to say?

[gavinking]

Oh wait, are you guys talking about a case where you have written @Id @GeneratedValue(name = "gitSeq"), but then never actually defined "gitSeq" anywhere using @SequenceGenerator?

I would have thought that that would be a hard error at startup. AFAIK, that isn't something that's allowed.

[gavinking]

@Sanne I feel like this is sort of a bug in Hibernate. The generator name is being interpreted as the name of a sequence, but I don't see how that interpretation is intended or implied by the JPA spec. Am I wrong?

[Sanne]

I agree @gavinking , I've also just learned via this issue that it doesn't fail hard - as I would also except.

on the other hand I see how it tries to get out of the way and make a reasonable assumption, and even if it's questionable I don't think we can "fix" this without breaking a lot of existing applications.

But in conclusion this is only a problem in combination with self-defined schemas.. and this is an area in which people need to take care for "manually": I'd say it's always going to be error prone, no matter what we do. Not sure about this being a bug.

Ideally, wondering if we can test the sequence state at initialization.

[gavinking]

Yup. I think we should go with your suggestion above and emit a warning, at least.

And if we had some sort of "strict" mode I would upgrade it to an error in H6.

[gavinking]

Ufff, I think I found something worse. According to the spec, generator names are global to a persistence unit, and the definition of a SequenceGenerator is shared between all the classes in the PU.

But according to my tests, in Hibernate 6 the generator name is local to the class the annotation occurs in, and can't be shared between entities or defined in a separate utility class.

(I haven't tested 5.5 yet.)

Am I understanding this correctly @sebersole?

[sebersole]

Starting in 5.3 we added a setting (`hibernate.jpa.compliance.global_id_generators`) to control whether those names are global or local. As you say JPA says they should be global regardless of placement. Our setting allows that to happen in a more natural (imo) paradigm. I am not sure I have tested that aspect in 6 yet.

…

On Sat, Aug 29, 2020 at 7:26 AM Gavin King ***@***.***> wrote: Ufff, I think I found something worse. According to the spec, generator names are global to a persistence unit, and the definition of a SequenceGenerator is shared between all the classes in the PU. But according to my tests, in Hibernate 6 the generator name is local to the class the annotation occurs in, and can't be shared between entities or defined in a separate utility class. (I haven't tested 5.5 yet.) Am I understanding this correctly @sebersole <https://github.com/sebersole>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11591 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZIE6FU3N5UWYQHJHQOCLSDDXVBANCNFSM4QKHQ74Q> .

[gavinking]

Ah perfect, thanks, I will try that out.

Indeed, I'm tempted to set that by default in Hibernate Reactive, WDYT?

[gavinking]

Oh wait you mean that in H5 the JPA-compliant behavior already is the default. OK that's good.

[Sanne]

FWIW I don't agree with the spec on this one and I'm happy we have the option to offer a better strategy.

It would be tempting to switch the Quarkus default to use the non-global definitions.

[gavinking]

I agree that the single global sequence thing is a bit strange.

[yrodiere]

It would be tempting to switch the Quarkus default to use the non-global definitions.

I think we did in ORM 6.0, at least to some extent?

https://github.com/hibernate/hibernate-orm/blob/6.0/migration-guide.adoc#implicit-identifier-sequence-and-table-name

Implicit Identifier Sequence and Table Name

The way in which Hibernate determines implicit names for sequences and tables associated with identifier generation has changed in 6.0 which may affect migrating applications.

To help with backwards compatibility, or to apply any general naming strategy, 6.0 introduces the org.hibernate.id.enhanced.ImplicitDatabaseObjectNamingStrategy contract which can be specified using the hibernate.id.db_structure_naming_strategy setting. See discussion at link:https://docs.jboss.org/hibernate/orm/6.0/javadocs/org/hibernate/cfg/AvailableSettings.html#ID_DB_STRUCTURE_NAMING_STRATEGY

For backwards compatibility, use either hibernate.id.db_structure_naming_strategy=single or hibernate.id.db_structure_naming_strategy=legacy depending on needs

By the way, heads-up to @Sanne: we may need to expose this configuration property in Quarkus 3 to provide a migration path for users who created their schema with Hibernate ORM 5? Maybe that should be part of a more general effort to provide a migration guide to users of the Hibernate ORM extension for Quarkus, because I suspect there are other breaking changes.

---

We got sidetracked, though. I must admit I'm not sure I understand the exact problem we need to solve in order to close this issue.

I think the conclusion was there:

(Sanne)

But in conclusion this is only a problem in combination with self-defined schemas.. and this is an area in which people need to take care for "manually": I'd say it's always going to be error prone, no matter what we do. Not sure about this being a bug.

Ideally, wondering if we can test the sequence state at initialization.

(Gavin)

Yup. I think we should go with your suggestion above and emit a warning, at least.

And if we had some sort of "strict" mode I would upgrade it to an error in H6.

Do I understand correctly that you would like to log a warning on startup (upon validating the schema?) when there is a @GeneratedValue(name = "gitSeq") without a corresponding @SequenceGenerator, and we cannot find the corresponding sequence in the schema?

[Sanne]

By the way, heads-up to @Sanne: we may need to expose this configuration property in Quarkus 3 to provide a migration path for users who created their schema with Hibernate ORM 5? Maybe that should be part of a more general effort to provide a migration guide to users of the Hibernate ORM extension for Quarkus, because I suspect there are other breaking changes.

Good point. Not sure about it being worth to itemize it all or simply go through the entire migration guide of ORM after we're done with the upgrade, and itemize+prioritize those - clearly we can't do it all but we should dedicate some time towards this. I'd not say it's a blocker for Quarkus 3 though, we can keep improving in subsequent minors.

We go sidetracked, though. I must admit I'm not sure I understand the exact problem we need to solve in order to close this issue.

Well you might say "sidetracked", I say "priorities" - the schema is wrong, we could improve the validation to be more user friendly but it doesn't feel super urgent to me, especially compared to other ongoing things.

Do I understand correctly that you would like to log a warning on startup (upon validating the schema?) when there is a @GeneratedValue(name = "gitSeq") without a corresponding @SequenceGenerator, and we cannot find the corresponding sequence in the schema?

Correct