hibernate_sequence considered harmful

[gavinking]

On databases which prefer sequence id generation, GenerationType.AUTO defaults to use of a single sequence with the name hibernate_sequence.

This is bad for two reasons:

1. pretty sure nobody actually wants a sequence named hibernate_sequence in their database, and
2. it means that the behavior on a platform which prefers sequences is quite different to the behavior on a platform which prefers identity columns: in one case the ids are unique at the table level, and in the other they're unique at the PU level.

Now, I need to do some more reading, but IMO, GenerationType.AUTO doesn't have any really well-defined semantics in JPA, and we can legitimately change its behavior to create a sequence based on the corresponding table name.

And I think we should do this in H6.

[sebersole]

To be clear... you are talking about just the name, right?

I bring this up because on the source issue in Quarkus there seemed to be some suggestions that we change the strategy that AUTO maps to, which I completely disagree with.

What you want is to change the strategy for changing the implicit name of the database structure backing the SequenceStyleGenerator, similar to SequenceStyleGenerator#CONFIG_PREFER_SEQUENCE_PER_ENTITY and SequenceStyleGenerator#CONFIG_SEQUENCE_PER_ENTITY_SUFFIX. You just want a different choice here.

What I would suggest is this... Take these settings I mentioned and extract them into a strategy (ImplicitGeneratorNamingStrategy ?) and provide the ability to plug-in alternatives. In fact I'd even go so far as to create short-named versions for a few different naming strategies.

Or maybe even a method on ImplicitNamingStrategy once we get that all hooked in properly. As I mentioned on the Quarkus issue, the big thing there is what to base the name on consistently considering that we have to account for entities as well as id-bags.

You sometimes like to scoff at backwards compatibility, and I am uber jealous that we cannot do that in ORM; but we can't - we have to consider backwards compatibility. We all agree the name sucks, but it has unfortunately sucked for the last 20 years. Whatever choice we make here needs to not break that.

Actually I think the existing settings may even be fine for your needs. The problem with extending that to your needs is that atm they are specific to each generator. We could look at a cascade setting: local > global. But that would give Quarkus, Panache and users the ability to control that for the whole SF (aside from local overrides).

So a few nice options I think.

And yes, I think we should finally change this in 6

[gavinking]

Question about that though: by default does AUTO use block allocation?

I guess I think it should.

[sebersole]

Good call. I thought it did already, but added a test and that is not what is happening currently. I don't think it is a great idea to make that change for 5.x and would add that to my vote proposal below for 6.x

[sebersole]

For posterity sake, the change in 5.x would be in org.hibernate.cfg.BinderHelper#makeIdGenerator, specifically the block:

if ( !isEmptyAnnotationValue( generatorName ) ) {
    ...
}

ATM there is no else block, but we'd simply add:

if ( !isEmptyAnnotationValue( generatorName ) ) {
    ...
}
else {
    // NOTE: we choose the constant from `SequenceStyleGenerator` but the parameter
    // key is actually the same for all generators that understand an allocation size
    params.setProperty( SequenceStyleGenerator.INCREMENT_PARAM, "50" );
}

[sebersole]

Workaround btw is to map this as:

@Id
@GeneratedValue( strategy = AUTO, generator = "sequence" )
@SequenceGenerator( name = "sequence" )

Not great to have to add the @SequenceGenerator but meh

[sebersole]

You just get the entity name used instead of the table name.

Entity name is fine.

Actually looking at that code for something else, we do in fact have the table name(s) available. This would be an even bigger user-facing change but we could decide to use the table name instead. In retrospect that would have been a better option

[gavinking]

So it seems the situation here is better than I thought. I had thought that AUTO sometimes meant autoincrement, but apparently that's not the case. And indeed, it seems a good thing that it's not the case. At least the behavior is pretty consistent across platforms.

Nevertheless, after sleeping on this, I still think that a sequence-per-hierarchy would be preferable to a single global sequence with a weird name. And I would still prefer to change it to work like that (by default) in H6.

WDYT, @sebersole?

[sebersole]

In isolation I totally agree that sequence-per-hierarchy (aka the existing "prefer sequence for entity" handling) would be the right choice.

I have the same concern I always have when we change things related to what we expect the user's relational schema to look like for the same config. To a large degree I can get behind this one though.

So personally I am fine with that. We can mention it in the migration guide.

Something we discussed today was a way to mark these discussions as "resolved" and going ahead with work on it. What we came up with in the absence of a proper "resolve" feature from GH is to vote to see if we have a consensus.

So let's call a vote on this, specifically the part about preferring sequence-per-hierarchy for AUTO.

There is a difficulty here though... atm (5.x) much of this is mucked up by OGM requirements. IMO there is a better solution for OGM and, TBH, I think we should consider stopping adding funky support for OGM and actually actively removing things like this that stand as a road-block to doing something better in ORM

All that said, I propose that we change the behavior to prefer preferring sequence-per-entity.

Additionally I propose that we clean up the fugliness here coming from the OGM "requirment", specifically:

1. drop IdGeneratorStrategyInterpreter (which was added for OGM) and simply expect that OGM would register a custom resolution with IdentifierGeneratorFactory
2. change the internal IdentifierGeneratorFactory#generatorStrategyToClassNameMap field from Map<String,Class> to some form of Collection of functional contract references based on GenerationType, IdentifierGeneratorDefinition, etc
3. change the contract of IdentifierGeneratorFactory#createIdentifierGenerator to pass along additional details like GenerationType and IdentifierGeneratorDefinition or pass them in the Map passed to #createIdentifierGenerator (though personally I very much dislike the "Map as an API" paradigm)

These additional (numbered) proposed changes simply make it easier to react to AUTO. As it stands atm, by the time we try to create the IdentifierGenerator the actual GenerationType is no longer available.

+1 / -1 ?

P.S. Per #3809 (reply in thread) I also think we should add the JPA default allocation/increment size of 50 when we have GenerationType#AUTO. This is a case where the proposed make this a lot easier/cleaner to achieve

[sebersole]

See:

• https://hibernate.atlassian.net/browse/HHH-14491
• https://hibernate.atlassian.net/browse/HHH-14492
• https://hibernate.atlassian.net/browse/HHH-14495
• https://hibernate.atlassian.net/browse/HHH-14496