Repository navigation
fix: rebuild lineage on PostgreSQL and keep entries on failure - #1664
theluckystrike wants to merge 3 commits into
Conversation
dimitri-yatsenko
left a comment
There was a problem hiding this comment.
Thanks for this. The full suite passes locally on MySQL and PostgreSQL, and the two new tests fail on master as you describe (3 failed, 1 passed).
One gap in keeping ~lineage on failure, noted inline.
| resolve_lineage(table, col) | ||
|
|
||
| # Replace the existing entries only now that every lineage has resolved | ||
| connection.query(f"DELETE FROM {lineage_table}") |
There was a problem hiding this comment.
Moving the DELETE here covers a key that cannot be resolved. But the DELETE and the insert below still run as separate statements outside a transaction, so if the insert fails (a dropped connection, for example), ~lineage is left empty, which is the state #1641 is about. Could the two run in one transaction?
One thing to watch: insert_lineages calls ensure_lineage_table, which runs CREATE TABLE IF NOT EXISTS. MySQL commits any open transaction before a CREATE TABLE, even when the table already exists, so calling insert_lineages inside with connection.transaction: would still commit the DELETE on its own. The table must already exist here, since the DELETE succeeded, so one way is to split the upsert out of insert_lineages into a helper that does not create the table, and run the DELETE and that upsert together:
with connection.transaction:
connection.query(f"DELETE FROM {lineage_table}")
if entries:
_upsert_lineages(connection, lineage_table, entries)A test could make the insert fail (for example with monkeypatch on the upsert) and check that the entries survive.
There was a problem hiding this comment.
Done in e9d2686, the way you sketched it. _upsert_lineages does the upsert without ensure_lineage_table, insert_lineages calls it after creating the table, and rebuild_schema_lineage runs the DELETE and the upsert inside with connection.transaction.
One addition. start_transaction raises on a nested transaction, so calling schema.rebuild_lineage() inside a transaction the caller already opened would start failing. In that case the rebuild reuses the open transaction instead of starting one.
Two new tests, on MySQL and PostgreSQL.
test_failed_lineage_insert_keeps_entriespatches_upsert_lineagesto raiseLostConnectionErrorand checks that~lineagestill holds the old entries and no transaction is left open. With the transaction removed it fails on both backends.test_rebuild_lineage_in_open_transactionruns a rebuild insidewith connection.transaction. Without the reuse it fails on both backends.
I ran the full suite locally against MySQL 8.4 and PostgreSQL 15 (1229 passed). The 12 failures there are 11 diagram tests (no Graphviz on this machine) and test_activate_unauthorized, and master fails the same 12 in the same setup.
rebuild_schema_lineage now runs the DELETE and the upsert together, so an insert that fails rolls the DELETE back. The upsert is split out of insert_lineages because ensure_lineage_table's CREATE TABLE would commit the transaction on MySQL. A rebuild inside a caller's open transaction reuses it.
dimitri-yatsenko
left a comment
There was a problem hiding this comment.
Thanks @theluckystrike. _upsert_lineages and the transaction around the DELETE and the upsert fix the case where the rebuild opens its own transaction: nothing inside it can commit early.
The new path that reuses a transaction the caller already opened still has the problem from my earlier review, on a different line. rebuild_schema_lineage calls ensure_lineage_table (CREATE TABLE IF NOT EXISTS) before it checks connection.in_transaction. Inside the caller's transaction, MySQL commits at that statement, even when the table exists. DataJoint still thinks it is in a transaction, so the DELETE and the upsert each commit on their own. For example:
with conn.transaction:
Animal.insert1(row)
schema.rebuild_lineage() # the upsert fails hereOn MySQL, the insert is already committed by the CREATE, ~lineage is left empty (the state #1641 describes), and the caller's rollback has nothing left to undo. test_rebuild_lineage_in_open_transaction checks only that no error is raised and the entries are unchanged, so it passes on MySQL anyway. PostgreSQL is fine, because its DDL runs inside the transaction.
The simplest fix is to refuse, as declare and alter already do inside a transaction (table.py, "Cannot declare new tables inside a transaction"). At the top of rebuild_schema_lineage, before ensure_lineage_table:
if connection.in_transaction:
raise DataJointError("Cannot rebuild lineage inside a transaction.")Then use a plain with connection.transaction: around the DELETE and the upsert, drop the contextlib import, and change test_rebuild_lineage_in_open_transaction to expect DataJointError inside with connection.transaction: and check that schema.lineage is unchanged afterward. On master, a rebuild inside a caller's transaction also committed that transaction on MySQL, so raising is no loss, only explicit.
If you'd rather keep rebuild working inside a caller's transaction, run the CREATE only when the table is missing, and refuse when it is missing and a transaction is open. That version needs a test proving the caller's rollback still works on MySQL: insert a row, rebuild, raise inside with conn.transaction:, then check that the row is gone and ~lineage is intact.
CREATE TABLE IF NOT EXISTS for ~lineage commits an open transaction on MySQL, so a rebuild inside the caller's transaction committed the caller's earlier writes and left the DELETE and upsert unprotected. Raise before any DDL, as declare and alter do. Signed-off-by: theluckystrike <51033404+theluckystrike@users.noreply.github.com>
|
Thanks @dimitri-yatsenko, you're right. The I took the simpler option in d59d4ec.
|
schema.rebuild_lineage()failed on PostgreSQL becauserebuild_schema_lineageread keys frominformation_schemawith MySQL only names (REFERENCED_TABLE_SCHEMAand friends, andCONSTRAINT_NAME = 'PRIMARY'). It had already runDELETE FROM ~lineageby then, so the failed call left the schema with an empty~lineagetable.The function now reads primary and foreign keys through
adapter.load_primary_keys_sqlandadapter.load_foreign_keys_sql, the same queriesDependencies.loaduses, and splits the returned names withadapter.split_full_table_name. Those foreign key queries also return keys that other schemas hold on this one, so rows whose referencing table is in another schema are ignored. TheDELETEmoved to just before the insert, after every lineage has resolved, so a rebuild that cannot resolve a key leaves~lineageas it was. That also covers the existing MySQL failure where an upstream schema has no lineage.Two tests were added to
tests/integration/test_multi_backend.py, and both run on MySQL and PostgreSQL.test_rebuild_lineageempties~lineageand checks the rebuilt entries for a parent and child table.test_failed_rebuild_lineage_keeps_entriesremoves the upstream lineage of a cross schema foreign key and checks that the downstream entries survive the error.I ran the tests against
postgres:15anddatajoint/mysql:8.0containers withDJ_USE_EXTERNAL_CONTAINERS=1. On master with the new tests,pytest tests/integration/test_multi_backend.py -k rebuildgave 3 failed and 1 passed (the MySQL happy path passes on master). With this change it gives 4 passed.pytest tests/integration/test_multi_backend.py tests/integration/test_semantic_matching.py tests/integration/test_dependencies.py tests/unitgave 512 passed.ruff checkandruff format --checkpass on the two changed files. I did not run the fullpixi run -e test testsuite or the S3 backed tests.Fixes #1641