possible bug in start_transaction (dbapi.py)

[bachng2017]

Issue:

start_transaction() (dbapi.py) might need to set the isolation level properly.

Details:

Currently the start_transaction does not change the isolation level (#b), while cursor() only compare the isolation level with AUTO_COMMIT(#a). The result is that the transaction and the query uses different request instance and run independently (perhaps)

• a. https://github.com/trinodb/trino-python-client/blob/master/trino/dbapi.py#L173
• b.

  trino-python-client/trino/dbapi.py

  Line 130 in c8144de

  def start_transaction(self):

For some application (likes dbt), the following sequence is heavily used:

1. start_transaction()
2. query
3. commit() or rollback()

If the query finishes within the transaction-timeout, everything looks fine. If the query succeed but took a longer time than transaction-timeout (default is 5 min), error will happen at 3 likes below. Note that in this case, the query succeed and the result is recorded (because it seems to run with auto_commit enabled sessions)

trino.exceptions.DatabaseError: failed to commit transaction 73dc7443-ec53-4911-95be-a372792743b4: TrinoUserError(type=USER_ERROR, name=UNKNOWN_TRANSACTION, message="Unknown transaction ID: 73dc7443-ec53-4911-95be-a372792743b4. Possibly expired? Commands ignored until end of transaction block", query_id=20211121_145311_00013_7rttf)

Expectation:

It might depend on the spec or definition of transaction-timeout, but expectation is that there is no error in the above mentioned case.

Suggested solution:

Maybe start_transaction should have the option to set isolation level (with REPEATABLE_READ as default). And commit()/rollback() need to set the isolation level back to the value it is set when connection is initialized.

[hovaesco]

Maybe start_transaction should have the option to set isolation level (with REPEATABLE_READ as default). And commit()/rollback() need to set the isolation level back to the value it is set when connection is initialized.

Thanks for rising the issue.

Would the suggested solution solve the issue when a transaction is executed > 5 min? I wonder how setting separate isolation level for start_transaction() help here.
IIUC, It's more about reusing the same transaction request instance in start_transaction() and commit/rollback().

@hashhar / @ebyhr / @findinpath any ideas?

[hashhar]

IIUC, It's more about reusing the same transaction request instance in start_transaction() and commit/rollback().

Correct. From my understanding there are two issues here:

1. Looks like we don't handle the case when connection is in auto-commit and the user explicitly manages their own transaction. The cause seems because we don't disable auto-commit when the cursor creates a transaction. See for reference Postgres driver - https://github.com/tlocke/pg8000/blob/a83e85b4a3493efe4a53fdbe142d53306c1f5622/pg8000/dbapi.py#L448-L449. And psycopg2 for example starts implicit transactions if connection isn't in autocommit.

2. transactions are broken in the client since we aren't managing the lifecycle of the request attached to a transaction correctly.

[bachng2017]

@hovaesco we did not test the case so it is just a suggestion. The reason I mentioned about isolation level is here

https://github.com/trinodb/trino-python-client/blob/master/trino/dbapi.py#L198

With correct isolation level, the request instance might be used correctly

I wonder how setting separate isolation level for start_transaction() help here.

[hovaesco]

Thanks @hashhar and @bachng2017 for the explanation. I'm starting to work to address this issue.

[hovaesco]

Closed by: #152