Conversation
exists is a method, so prior to change, assert would always pass thankfully, this still works on sqlalchemy 1.4 after change
seems that table.table.exists() actually created table if we replace it with self.db_has_table instead, it doesn't work I assume because it works on sqlalchemy metadata level and not issuing actual sql engine commands to demonstrate that I've put the asserts with self.db.has_table in setUp method for now just removed the assert from test_create_table_shorthand* tests
passes all tests on sqlalchemy 1.4
all tests passed
| row = self._sync_columns(row, ensure, types=types) | ||
| res = self.db.executable.execute(self.table.insert(row)) | ||
| res = self.db.conn.execute(self.table.insert(), row) | ||
| # if not self.db.in_transaction: |
There was a problem hiding this comment.
Shouldn't db.commit() be called here? Otherwise the transaction stays open (started by db.conn on first connect) until the user calls db.commit(). Perhaps instead of calling db.conn.execute() directly, there should be a db.execute() that handles transactions implicitly, i.e. distinct between user created transactions and autocommit.
| return self.local.conn | ||
| except AttributeError: | ||
| self.local.conn = self._engine.connect() | ||
| self.local.conn.begin() |
There was a problem hiding this comment.
it seems calling conn.begin() here means that there is only ever one transaction opened, even in presence of multiple table.insert() calls.
IMHO it would be better to have an explicit semantic where db.conn just returns the connection and transaction context is handled by the respective operations (insert, etc.). There should be a distinction between the autocommit case (user did not request a transaction) and the explicit commit case (user started the transaction).
|
|
||
| No data will be written until the transaction has been committed. | ||
| """ | ||
| if not hasattr(self.local, "tx"): |
There was a problem hiding this comment.
wouldn't this remove support for nested transactions? as in https://dataset.readthedocs.io/en/latest/quickstart.html#using-transactions
|
Dear @pudo, we absolutely love this package, and would like to send kudos to @karlicoss, @M4rque2, and you for conceiving and wrapping up this patch. Also, we would like to kindly ask if this patch can be integrated, or if any support is needed on it? With kind regards, |
There is an issue with concurrent transactions occurring, causing a deadlock in some scenarios. I have tried to resolve it a couple of weeks back, applied some updates, but got stuck in exactly the same place. Didn't have time to figure out the root cause yet. |
5 participants
Final changes to #415 , fixes #412