Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quarkus 3 update script: ERROR: relation "hibernate_sequence" does not exist #32988

Open
kdubois opened this issue Apr 28, 2023 · 15 comments
Open
Labels
area/upgradetool issues related to upgrade tooling (openrewrite, windup, cli, etc.) kind/enhancement New feature or request

Comments

@kdubois
Copy link
Contributor

kdubois commented Apr 28, 2023

Description

Not sure how, but it would be nice if the quarkus update --stream=3.0 script could add create sequence hibernate_sequence start with 1 increment by 1; preceding import statements since the previous version of Hibernate would allow you to do statements such as:

INSERT INTO Fruit(id,name,season) VALUES (nextval('hibernate_sequence'),'Mango','Spring');

which now error out with: relation "hibernate_sequence" does not exist

Implementation ideas

No response

@kdubois kdubois added the kind/enhancement New feature or request label Apr 28, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 28, 2023

/cc @Sanne (hibernate-orm), @gsmet (hibernate-orm), @yrodiere (hibernate-orm)

@yrodiere yrodiere added area/upgradetool issues related to upgrade tooling (openrewrite, windup, cli, etc.) and removed area/persistence area/hibernate-orm Hibernate ORM labels Apr 28, 2023
@yrodiere
Copy link
Member

I don't think this is the right solution, as Hibernate ORM 6 will expect one sequence per entity type.

Have you read the migration guide, in particular this section?

@kdubois
Copy link
Contributor Author

kdubois commented Apr 28, 2023

I read it, albeit briefly :D It was just a suggestion of a quick way of solving a simple set of insert statements that are using hibernate_sequence... But I'm not sure what the impact would be if you had a more elaborate import script. Maybe this is just one of those things that can't be solved by script, but in that case, it would be nice to return a string or text file with actions the user should take care of manually?

@yrodiere
Copy link
Member

yrodiere commented Apr 28, 2023

It was just a suggestion of a quick way of solving a simple set of insert statements that are using hibernate_sequence

The thing is, with this solution, you script will use hibernate_sequence but your application will still use per-entity sequences. And you will end up with a hibernate_sequence in your schema that your application will never use.

So, I don't think this is actually a solution.

in that case, it would be nice to return a string or text file with actions the user should take care of manually?

The errors are returned by your database, not by Hibernate ORM or Quarkus.

Besides, those actions are basically the migration guide I pointed to.

Maybe this is just one of those things that can't be solved by script

I would agree. There are only two approaches, and neither can (easily) be automated:

  • Adapt your database schema to match what Hibernate ORM expects (which seems to be what you did, by letting Hibernate ORM create your database schema), and thus change your migration script to use the sequences that Hibernate ORM expects.
  • OR adapt your annotations to use a single sequence across all your entities, using @GeneratedValue(generator = "mysinglesequence") and a custom sequence generator definition (@SequenceGenerator,(name = "mysinglesequence", sequence = "hibernate_sequence", allocationSize = 1)), and thus keep your migration script as is.

The only (temporary) automated solution you can use is the Hibernate ORM 5.6 compatibility switch, which will force Hibernate ORM 6 to intepret (most of) your annotations like Hibernate ORM 5.6 did. But that's just delaying the problem: ORM 5.6 compatibility won't be around forever.

@yrodiere
Copy link
Member

yrodiere commented May 2, 2023

I'm going to close this issue since I don't think we can do more than document the steps to go through in the migration guide.

Feel free to add another message and ping me if you still think we could still do something more in Quarkus despite my explanation above.

@yrodiere yrodiere closed this as not planned Won't fix, can't repro, duplicate, stale May 2, 2023
@maxandersen maxandersen reopened this May 28, 2023
@maxandersen
Copy link
Contributor

@yrodiere in context of the quarkus update that is trying to move existing users from quarkus 2 to quarkus 3 we must try do better than just leave people with a broken app.

at least make the update script print or generate a warning somewhere as afaics we should be able to detect the issue will be there.

wdyt?

@maxandersen
Copy link
Contributor

@yrodiere
Copy link
Member

as afaics we should be able to detect the issue will be there.

Statically, just from quarkus update? I'm not sure, at least not easily or imperfectly. Unless you're fine with printing the warning all the time.

Alternatively we could set the Hibernate ORM 5.6 compatibility switch when someone runs quarkus update, but since we did discuss it at some point, I suppose there is a reason it wasn't done... any insight @gsmet?

@maxandersen
Copy link
Contributor

i wouldn't necessarily set the compatibility switch - suggesting that specifically for "nextval('hibernate_sequence')" antipattern in import.sql we could do the transformation?

@maxandersen
Copy link
Contributor

actually, I guess if we see nextval('hibernate_sequence") we could add the compatability switch? then its not added "blindly" at least?

@yrodiere
Copy link
Member

yrodiere commented May 30, 2023

suggesting that specifically for "nextval('hibernate_sequence')" antipattern in import.sql we could do the transformation?

If you're suggesting to parse SQL, understand it and transform it, well... I feel like I'll just state the obvious (especially since you're familiar with the matter), but it's not a simple task.

Let's assume we can execute arbitrary Java code in the upgrade tool and parse the SQL properly using Hibernate ORM's own parsers (which are not perfect, but they're the ones used to parse import.sql, so they're a good fit).

Then we'd replace nextval('hibernate_sequence') with...

  1. Automatically generated hardcoded numbers? Probably considered icky despite it being the good practice, but that means resetting the sequences to max(id) afterwards. I won't even mention entities that use an UUID instead of a number :)
  2. The right name for the sequence? That means understanding which table the statement is inserting to, somehow building the ORM metadata to map that table name back to the corresponding entity, then getting the root entity type, then retrieving the expected name of the corresponding sequence.

And in both cases, we'll get in a ton of trouble if nextval is actually used in a trigger or stored procedure.

Sure we can pile on exceptions like "don't try to migrate the script if you see there's a stored procedure being declared" but... is all this really worth it, just to spare users the task of googling?


actually, I guess if we see nextval('hibernate_sequence") we could add the compatability switch? then its not added "blindly" at least?

There would be false positives for sure, but I suppose they would be rare.

I may be wrong but I remember reading that the tooling wasn't necessarily flexible enough to check the presence of a string in a file and react by adding something in another file.

Only hearsay though, we need someone who works on this.


Regardless... I agree the migration is complex and we should help if we can, but IMO we should be careful not to make the upgrade easier for some users and much harder (because we didn't guess right) for other users...

"Guessing right" is the at the core of the issue here. We could try to be conservative but I doubt we'll help many people that way. IMO we should try to minimize the effect of false positives instead, and the best way to do that would probably to avoid automated changes and to stick to displaying warnings.

@holly-cummins
Copy link
Contributor

I have a bad feeling that if we can't do the right thing in the automated tooling (and I appreciate its complex), some of our users might have even less chance of being able to do the right thing. I base this entirely on my own experience. :)

One thing that would help is a few examples in the docs to copy and paste from. I was able to make my upgrade work by finding someone who'd done a similar update and copying their code. There won't be a single copy-pastable thing that works for all cases, but if we have a few examples of different things that might work, it's something for users to experiment with. It's also something concrete that they can feed into their favourite search engine to understand what the code snippets are showing.

@maxandersen
Copy link
Contributor

I wouldn't even go as far as make it parse the sql - but just look for common pattern - but I agree can be a bit too faulty.

btw. openrewrite plugins can do anything. just that someone need to write those plugins.

This is a bit of a special case but it would be nice to even just have the update tooling detect "hey, import.sql is referring to hibernate_sequence. You might need to change that to a sequence per table. Link to more docs" and add that to a "read_afterupdate.md" file or similar - as we might not do perfect migrations but can at least detect possible issues and recommend/document options.

@Sanne
Copy link
Member

Sanne commented May 31, 2023

This is a bit of a special case but it would be nice to even just have the update tooling detect "hey, import.sql is referring to hibernate_sequence. You might need to change that to a sequence per table. Link to more docs" and add that to a "read_afterupdate.md" file or similar - as we might not do perfect migrations but can at least detect possible issues and recommend/document options.

I love this idea.

Definitely -1 to try automagically patch it though - it's too dependent on actual database structure and expectations, which we can't infer, not to mention SQL parsing like Yoann mentioned is a pandora's box.

The general pattern I see is emotional though.. people expect the Hibernate ORM upgrade to be rather simple and then get scared + severely concerned about their future.

IMO the best we can do is keep pressing on this message that it's not a walk in the park, to get people to understand they'll need to set some time aside for it, and spend some quality time actually reading the migration guides without thinking they'll "figure it out", possibly even doing some new POCs to understand some core differences better before migrating a non trivial project.

@Sanne
Copy link
Member

Sanne commented May 31, 2023

BTW I've never in my life depended on "hibernate_sequence" in my own scripts. This was a somewhat fancy idea that someone among us had to do a clever demo and it kinda became a pattern that every other demo and quickstart copied - but I don't expect it to be a wide-spread pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/upgradetool issues related to upgrade tooling (openrewrite, windup, cli, etc.) kind/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants