[Doctrine][Messenger] Use common sequence name to get id from Oracle

[rjd22]

Q	A
Branch?	6.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix #54193
License	MIT

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.

[derrabus]

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]

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]

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

[nicolas-grekas]

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

[rjd22]

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

@rjd22 Any news?

[rjd22]

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

[rjd22]

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

Would someone be willing to take a look for me?

[rjd22]

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

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]

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]

For manual migration the users will have to run the following queries (please adapt to table name if using a different table name):

create sequence seq_messenger_messages

alter table messenger_messages modify id default "seq_messenger_messages.nextval"

[rjd22]

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

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

[xabbuh]

works for me

[fabpot]

Thank you @rjd22.