Open Glandos opened 2 years ago
Thanks for the investigation! I'm inclined to agree with zzzeek's comment that Sequence shouldn't be needed, but removing existing sequences will affect migrations, tests, etc. In the long-term, it might be better to wait for the MySQL dialect to support returning
.
I think we might be able to reference context.invoked_statement.table.name
and similar instead of parsing the generated SQL. However, we still ideally want to skip the nextval
query (and any others) and it's not clear to me yet how to do that.
As you already guessed, using table_name = context.invoked_statement.table.name
makes our complete test suite pass. But I'm not sure that we are using "advanced" features like bulk insert.
Do you think this small change could be integrated, as it obviously seems a better thing to do than splitting statement string?
That's good to know! I just need to check whether we need to deduplicate the operations (we might be inserting twice per transaction with this change). I've run out of time this evening but will try to take a look tomorrow.
Just to update here, substituting that line should be safe for typical uses, when the only table with a sequence is Continuum's Transaction table. The scenarios that may not be safe are when an association table (one used in a relationship with secondary
) has an explicit sequence or schema. I think I need to write a test around this before I'm comfortable committing it.
Hey, I'm not sure what to do about this. Is it fixed? Needs some work? Thanks!
For now, I've monkeypatched, but this is quite ugly: https://github.com/spiral-project/ihatemoney/blob/master/ihatemoney/monkeypath_continuum.py#L40
I had to copy/paste the whole factory :/
Having rewritten these tests for 2.0, I've realised that an association table with a sequence on shouldn't happen. So could you try the new sqla-2.0 branch? It should support 1.4 and above.
We encountered a similar problem using Microsoft SQL Server. We are getting the following error message:
ProgrammingError: (pyodbc.ProgrammingError) ('42S02', "[42S02] [Microsoft][ODBC Driver 18 for SQL Server][SQL Server]Invalid object name 'transaction_id_seq'. (208) (SQLExecDirectW); [42S02] [Microsoft][ODBC Driver 18 for SQL Server][SQL Server]Statement(s) could not be prepared. (8180)")
[SQL: INSERT INTO [transaction] (id, remote_addr, issued_at) OUTPUT inserted.id VALUES (NEXT VALUE FOR transaction_id_seq, ?, ?)]
Are there any updates regarding this issue?
Versions we use: SQLAlchemy - 2.0.19 SQLAlchemy-Continuum - 1.4.0
In https://github.com/spiral-project/ihatemoney/ we still support MariaDB, and are trying to upgrade to SQLAlchemy 1.4 However, CI tests are failing with MariaDB 10.3 with the following log:
The log is quite extensive, so I asked the question to the SQLAlchemy community on https://github.com/sqlalchemy/sqlalchemy/discussions/7649
The maintainer answer led me to https://github.com/kvesteri/sqlalchemy-continuum/blame/2e53b5f927c7b601c1dab02b2c91c4dbb3b9ed15/sqlalchemy_continuum/transaction.py#L125 (but there is also https://github.com/kvesteri/sqlalchemy-continuum/blame/2e53b5f927c7b601c1dab02b2c91c4dbb3b9ed15/sqlalchemy_continuum/plugins/activity.py#L204) were explicit
Sequence()
are used, since the merge of https://github.com/kvesteri/sqlalchemy-continuum/commit/0bb36e02b9db7b65133c037ca993effbc78c299b. This was based on work in https://github.com/kvesteri/sqlalchemy-continuum/pull/118 that mentioned support for Oracle database.According to the SQLAlchemy maintainer:
I tried to set
optional=True
as mentioned in https://github.com/kvesteri/sqlalchemy-continuum/pull/118#issuecomment-215553949 but this doesn't solve the issue. However, commenting the line solved this issue. So I have to request to change something here. Obviously the support for Oracle is needed, so maybe the sequence should be defined only when this backend is in use. I didn't look (yet?) intoIDENTITY
as mentioned, but I can't test Oracle by myself, so if someone has a clue on this, I'll be delighted.In the meantime we may monkey-patch or use another transaction factory.