[Django] #31320: Prevent BEGIN and COMMIT in RunSQL in atomic migrations
Django
-------------------------------------------------+------------------------
Reporter: Adam (Chainz) Johnson | Owner: nobody
Type: New feature | Status: new
Component: Migrations | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------------+------------------------
Migrations are `atomic` by default, which means they use a transaction on
backends that support them (PostgreSQL).
`RunSQL` allows running arbitrary SQL, including `BEGIN` / `COMMIT` which
break the atomic transaction handling.
PostgreSQL will warn about `BEGIN` within a transaction (
https://www.postgresql.org/docs/current/sql-begin.html ) but this warning
won't be displayed on a standard Django setup. Equally it will warn about
a `COMMIT` when no transaction is running.
(SQLite at least raises an error for a `BEGIN` within a transaction).
Beginners to the migration framework can miss that migrations are 'atomic'
by default, and use their SQL knowledge to write `BEGIN`/`COMMIT`, not
realizing they're actually breaking the transactional integrity of their
migrations.
It would be good to detect this and refuse to run such `RunSQL`
operations.
--
Ticket URL: <https://code.djangoproject.com/ticket/31320>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: New feature | Status: closed
Component: Migrations | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: new => closed
* resolution: => wontfix
Comment:
Thanks for this ticket, however `RunSQL()` is ''"useful for more advanced
features of database backends that Django doesn’t support directly"'',
moreover you can use `BEGIN` not only to start a transaction, e.g. with
[https://www.postgresql.org/docs/9.6/sql-do.html DO]. IMO parsing a custom
SQL provided by users in `RunSQL()` is out of the scope of Django.
--
Ticket URL: <https://code.djangoproject.com/ticket/31320#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: New feature | Status: closed
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
I'm pretty sure sqlparse parses the `BEGIN` statement differently ot the
keyword in `DO ... BEGIN .. END`. And we already parse the SQL in RunSQL
for splitting, so it doesn't seem like it would be much overhead to me to
make a pass over the tokens at that point and warn/error. Migrations using
`BEGIN` / `COMMIT` could possibly cause data loss so I think it's a fair
case to guard against.
--
Ticket URL: <https://code.djangoproject.com/ticket/31320#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: New feature | Status: closed
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* cc: Simon Charette (added)
Comment:
> And we already parse the SQL in RunSQL for splitting, so it doesn't seem
like it would be much overhead to me to make a pass over the tokens at
that point and warn/error.
Yes, but not on PostgreSQL. For a long statements this can introduce a
performance regression, IMO. Do you want to prepare a patch? I'm not
convinced that we need to be more protective in `RunSQL()`, I've used it
many times but always for a complicated data migrations.
--
Ticket URL: <https://code.djangoproject.com/ticket/31320#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: New feature | Status: closed
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
I've gone for a docs PR instead:
https://github.com/django/django/pull/12511
The reason I made this was when squashing migrations on a client project,
it had some RunSQL operations in migrations that worked fine alone, but
when the operatiosn were squashed together it broke the squashed
migration's transactional state.
--
Ticket URL: <https://code.djangoproject.com/ticket/31320#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: New feature | Status: closed
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
> The reason I made this was when squashing migrations on a client
project, it had some RunSQL operations in migrations that worked fine
alone, but when the operatiosn were squashed together it broke the
squashed migration's transactional state.
Interesting, were these migrations marked `atomic = False` initially? It
looks like
[https://github.com/django/django/blob/2bd8df243ac6fc35e58c9fe90b20c9e42519a5ac/django/core/management/commands/squashmigrations.py#L165-L170
the current code] completely ignores this flag. I guess the command could
at least warn about it.
Migration squashing is definitely an area that would benefit from a bit of
love wrt to dependency resolving and could be a good GSOC project in the
next years if we ever apply.
From [https://docs.djangoproject.com/en/3.0/topics/migrations/#migration-
squashing the docs themselves]
> To manually resolve a CircularDependencyError, break out one of the
ForeignKeys in the circular dependency loop into a separate migration, and
move the dependency on the other app with it. If you’re unsure, see how
makemigrations deals with the problem when asked to create brand new
migrations from your models. **In a future release of Django,
squashmigrations will be updated to attempt to resolve these errors
itself.**
It looks like non-atomic migrations pose a similar problem where they'd
need to be considered an optimization barrier and require the creation of
a standalone migration.
--
Ticket URL: <https://code.djangoproject.com/ticket/31320#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: New feature | Status: closed
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
> Interesting, were these migrations marked atomic = False initially?
No
> It looks like non-atomic migrations pose a similar problem where they'd
need to be considered an optimization barrier and require the creation of
a standalone migration.
Yes, that would be great
--
Ticket URL: <https://code.djangoproject.com/ticket/31320#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: New feature | Status: closed
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by GitHub <noreply@…>):
In [changeset:"e9b014fbc56b9baf91019a803ab2a45788c5c44a" e9b014f]:
{{{
#!CommitTicketReference repository=""
revision="e9b014fbc56b9baf91019a803ab2a45788c5c44a"
Refs #31320 -- Warned against using BEGIN/COMMIT in RunSQL.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31320#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: New feature | Status: closed
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"c9437596fe54fb5cf0330ee5a96e260903a2d683" c9437596]:
{{{
#!CommitTicketReference repository=""
revision="c9437596fe54fb5cf0330ee5a96e260903a2d683"
[3.0.x] Refs #31320 -- Warned against using BEGIN/COMMIT in RunSQL.
Backport of e9b014fbc56b9baf91019a803ab2a45788c5c44a from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31320#comment:8>