Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
fabpot merged 1 commit intosymfony:6.4fromrjd22:fix/oracle-sequence-support
Oct 6, 2024

Conversation

@rjd22
Copy link
Contributor

QA
Branch?6.4
Bug fix?yes
New feature?no
Deprecations?no
IssuesFix#54193
LicenseMIT

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.

@rjd22rjd22force-pushed thefix/oracle-sequence-support branch fromefe7c35 to8fd6ff7CompareApril 11, 2024 08:39
@rjd22rjd22 changed the titlefix #54193: use common sequence name to get id from Oraclefix #54193: use common sequence name to get id from Oracle for MessengerApr 11, 2024
@rjd22rjd22 changed the titlefix #54193: use common sequence name to get id from Oracle for Messenger[Messenger] fix #54193: use common sequence name to get id from OracleApr 11, 2024
@rjd22rjd22 changed the title[Messenger] fix #54193: use common sequence name to get id from Oracle[Messenger] Use common sequence name to get id from OracleApr 11, 2024
@derrabus
Copy link
Member

This will require the user to name their sequences seq_<table_name> or SEQ_<TABLE_NAME>

Do we create this sequence during auto-setup?

@rjd22
Copy link
ContributorAuthor

rjd22 commentedApr 11, 2024
edited
Loading

This will require the user to name their sequences seq_<table_name> or SEQ_<TABLE_NAME>

Do we create this sequence during auto-setup?

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.

@nicolas-grekas
Copy link
Member

It'd be great to fix the autosetup and let doctrine manage the sequence.

@nicolas-grekasnicolas-grekas added this to the6.4 milestoneApr 11, 2024
@nicolas-grekas
Copy link
Member

#54176 might be trying to fix the same issue. Can you please have a look in case this brings inspiration?

@rjd22
Copy link
ContributorAuthor

#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.

@fabpot
Copy link
Member

@rjd22 Any news?

@rjd22
Copy link
ContributorAuthor

@fabpot at this moment this is pending. I need to make some time to finish this and test this.

@rjd22rjd22force-pushed thefix/oracle-sequence-support branch from8fd6ff7 toe799d36CompareSeptember 13, 2024 11:48
@rjd22
Copy link
ContributorAuthor

@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.

@rjd22
Copy link
ContributorAuthor

Would someone be willing to take a look for me?

@rjd22
Copy link
ContributorAuthor

@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.

@xabbuh
Copy link
Member

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
Copy link
ContributorAuthor

rjd22 commentedSep 24, 2024
edited
Loading

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?

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 :).

@rjd22
Copy link
ContributorAuthor

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"

@rjd22
Copy link
ContributorAuthor

@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.

@rjd22
Copy link
ContributorAuthor

@xabbuh well not much interest to test it sadly :(. Would it be okay to merge this?

Copy link
Member

@xabbuhxabbuh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

works for me

@carsonbotcarsonbot changed the title[Messenger] Use common sequence name to get id from Oracle[Doctrine][Messenger] Use common sequence name to get id from OracleOct 1, 2024
@fabpotfabpotforce-pushed thefix/oracle-sequence-support branch frome799d36 to52c3a32CompareOctober 6, 2024 09:09
@fabpot
Copy link
Member

Thank you@rjd22.

@fabpotfabpot merged commitc898802 intosymfony:6.4Oct 6, 2024
9 of 10 checks passed
This was referencedOct 27, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@xabbuhxabbuhxabbuh approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

6.4

Development

Successfully merging this pull request may close these issues.

6 participants

@rjd22@derrabus@nicolas-grekas@fabpot@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp