Opened 14 months ago

Closed 14 months ago

Last modified 14 months ago

#22496 closed Bug (fixed)

Data migrations are not transactional (except on Postgres)

Reported by: shai Owned by: nobody
Component: Migrations Version: master
Severity: Release blocker Keywords: oracle mysql sqlite
Cc: aaugustin, andrewgodwin Triage Stage: Accepted
Has patch: no Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In e74d2183c28467aefc0b87e3fa6d405dbfdea82c (master) and 9bf890f6f9d137d5b86fd9a6a38fb11c5d21b1af (1.7.x) schema editor was made not to open transactions for backends that cannot roll back DDL. However, there is nothing else that starts transactions in migrations, even if they are data migrations -- so now, of the backends in core, migrations are only transactional on PostgreSQL.

This is a spin-off from #22483.

Change History (6)

comment:1 Changed 14 months ago by aaugustin

First, we should set can_rollback_ddl = True for SQLite. SQLite fully supports transactional DDL, even thouh the Python sqlite3 module goes to great lengths to prevent developers from using that capability. With the new transaction model introduced in Django 1.6, I believe this change is safe.


This leaves MySQL and Oracle. Let's consider special operations:

  • RunPython should generally run in a transaction, but I can imagine use cases where it would run DDL statements through cursor.execute, among other things.
  • RunSQL is a problem: what if it runs some custom DDL statements?
  • I'm not sure about SeparateDatabaseAndState.

In fact the real question is the failure mode when running DDL in a transaction. If the transaction simply gets committed, it isn't great (because of the mismatch between the actual transaction state and what Django believes the state is) but it's acceptable. If Django crashes, it's bad :-) We may get bitten by "Oracle needs to close and reopen the connection after DDL" again...

If we decide to make these operations transactional, perhaps that could be controlled by an atomic flag or keyword argument to the Operation class. (Looking at the code, it would require a refactoring.) A simpler solution would be to decorate database_forwards/backwards with @atomic but that would make subclassing more difficult to get right.

The alternative is to leave transaction management entirely up to the user. But users aren't good at transaction management in general.

If I had to choose, I would go for an atomic flag that's enabled by default for the three special operations but can be turned off by users who're running into one of the edge cases where the transaction prevents the operation from executing.

comment:2 Changed 14 months ago by Aymeric Augustin <aymeric.augustin@…>

In e368912902add8accf08d290ea77df49bd488744:

Set some transaction-related feature flags on SQLite.

Refs #22496.

comment:3 Changed 14 months ago by Aymeric Augustin <aymeric.augustin@…>

In 782fa14db4b855b4c5034413d1065ea0915f6491:

[1.7.x] Set some transaction-related feature flags on SQLite.

Refs #22496.

Backport of e368912 from master.

comment:4 Changed 14 months ago by andrewgodwin

My vote is that we definitely leave RunSQL as non-transactional on MySQL and Oracle (SeparateDatabaseAndState is a combiner, it should be left alone too).

I also agree that we should have RunPython in a transaction, but have an argument to turn it off if you're doing schema changes on Oracle (the only know crash case I know of). I'll commit that now and mark this as fixed.

comment:5 Changed 14 months ago by Andrew Godwin <andrew@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 5a917cfef319df33ca30a3b27bd0b0533b8e63bb:

Fixed #22496: Data migrations get transactions again!

comment:6 Changed 14 months ago by Andrew Godwin <andrew@…>

In 7f63ac5a9f0ed76d2e372a8c0fda95cce67e7170:

[1.7.x] Fixed #22496: Data migrations get transactions again!

Note: See TracTickets for help on using tickets.
Back to Top