Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Doctrine][Messenger] Use common sequence name to get id from Oracle#54566
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
efe7c35 to8fd6ff7Compare
Do we create this sequence during auto-setup? |
rjd22 commentedApr 11, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'm not sure auto setup works for Oracle. I had to setup the tables manually for them to work properly. Edit: retested the auto create and it indeed fails with an error because of the json data type. |
It'd be great to fix the autosetup and let doctrine manage the sequence. |
#54176 might be trying to fix the same issue. Can you please have a look in case this brings inspiration? |
That PR solves a different issue that is not really an issue for Oracle versions higher than 12c. You can link the sequence to the id column as identity and it will auto increment the ID column just like the other databases. This PR solves the part to get the currval of the just inserted ID. I rather not change the method of insertion of the record because that would divert too much from the other implementations. Still there is some nice inspiration in that PR. And I will check if I can make the auto_setup work. |
@rjd22 Any news? |
@fabpot at this moment this is pending. I need to make some time to finish this and test this. |
8fd6ff7 toe799d36Compare@fabpot Sorry it took me longer than expected. These changes should solve the messenger issues with Oracle and also help new users to setup the messenger table correcty. We could improve on this by splitting all Oracle changes off to an separate OracleConnection (like postgres) but this would also mean quite a rewrite of the Connection itself. |
Would someone be willing to take a look for me? |
@nicolas-grekas Sorry for pinging but can you mark this for review? This fixes quite a important block of an upgrade path for Oracle users using messenger. |
The change itself looks good to me. Works be great to have some Oracle users give their feedback though. What I wonder though is: Will this break existing setups is they don’t add the sequence manually? |
rjd22 commentedSep 24, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yes and no, all existing setups are already broken after 6.4.3 and to upgrade they have to manually migrate the table or remove it and make the script recreate it. The ones that already have a sequence connected with the correct name (I tried to use the most common name for it). Should start working correctly. Because of licensing Oracle testers are really hard to come by :). |
For manual migration the users will have to run the following queries (please adapt to table name if using a different table name): createsequenceseq_messenger_messages altertable messenger_messages modify id default"seq_messenger_messages.nextval" |
@xabbuh lets wait for this week and if noone comes forward I recommend merging this and testing this in the field. If any things pop up then we will fix them. Atleast we have the advantage here that we can't make it worse. |
@xabbuh well not much interest to test it sadly :(. Would it be okay to merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
works for me
e799d36 to52c3a32CompareThank you@rjd22. |
c898802 intosymfony:6.4Uh oh!
There was an error while loading.Please reload this page.
This is a fix for#54193 by using a common sequence name for getting the ID's for Oracle databases.
This will require the user to name their sequences seq_<table_name> or SEQ_<TABLE_NAME> but at least allows you to use messenger with Oracle after properly setting up the table.