[Django] #30189: sqlmigrate wraps it's outpout in BEGIN/COMMIT even if the database doesn't support transactional DDL
Django
support transactional DDL
------------------------------------------+------------------------
Reporter: Simon Charette | Owner: nobody
Type: Bug | 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: 1
UI/UX: 0 |
------------------------------------------+------------------------
The migration executor only adds the outer `BEGIN`/`COMMIT`
[https://github.com/django/django/blob/edec11ce86a1a0d9e4c5a2a0df6acaf655041c24/django/db/migrations/executor.py#L243-L247
if the migration is atomic] and
[https://github.com/django/django/blob/edec11ce86a1a0d9e4c5a2a0df6acaf655041c24/django/db/backends/base/schema.py#L97
the schema editor can rollback DDL] but the current `sqlmigrate` logic
only takes `migration.atomic` into consideration.
The issue change be addressed by
1. Changing `sqlmigrate`
[https://github.com/django/django/blob/edec11ce86a1a0d9e4c5a2a0df6acaf655041c24/django/core/management/commands/sqlmigrate.py#L59
assignment] of `self.output_transaction` to consider
`connection.features.can_rollback_ddl` as well.
2. Adding a test in `tests/migrations/test_commands.py` based on
[https://github.com/django/django/blob/2bd8df243ac6fc35e58c9fe90b20c9e42519a5ac/tests/migrations/test_commands.py#L606-L617
an existing test for non-atomic migrations] that mocks
`connection.features.can_rollback_ddl` to `False` instead of overdidding
`MIGRATION_MODULES` to point to a non-atomic migration.
I marked the ticket as ''easy picking'' because I included the above
guidelines but feel free to uncheck it if you deem it inappropriate.
--
Ticket URL: <https://code.djangoproject.com/ticket/30189>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
support transactional DDL
Reporter: Simon Charette | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Description changed by Simon Charette:
Old description:
> The migration executor only adds the outer `BEGIN`/`COMMIT`
> [https://github.com/django/django/blob/edec11ce86a1a0d9e4c5a2a0df6acaf655041c24/django/db/migrations/executor.py#L243-L247
> if the migration is atomic] and
> [https://github.com/django/django/blob/edec11ce86a1a0d9e4c5a2a0df6acaf655041c24/django/db/backends/base/schema.py#L97
> the schema editor can rollback DDL] but the current `sqlmigrate` logic
> only takes `migration.atomic` into consideration.
>
> The issue change be addressed by
>
> 1. Changing `sqlmigrate`
> [https://github.com/django/django/blob/edec11ce86a1a0d9e4c5a2a0df6acaf655041c24/django/core/management/commands/sqlmigrate.py#L59
> assignment] of `self.output_transaction` to consider
> `connection.features.can_rollback_ddl` as well.
> 2. Adding a test in `tests/migrations/test_commands.py` based on
> [https://github.com/django/django/blob/2bd8df243ac6fc35e58c9fe90b20c9e42519a5ac/tests/migrations/test_commands.py#L606-L617
> an existing test for non-atomic migrations] that mocks
> `connection.features.can_rollback_ddl` to `False` instead of overdidding
> `MIGRATION_MODULES` to point to a non-atomic migration.
>
> I marked the ticket as ''easy picking'' because I included the above
> guidelines but feel free to uncheck it if you deem it inappropriate.
New description:
The migration executor only adds the outer `BEGIN`/`COMMIT`
[https://github.com/django/django/blob/edec11ce86a1a0d9e4c5a2a0df6acaf655041c24/django/db/migrations/executor.py#L243-L247
if the migration is atomic] and
[https://github.com/django/django/blob/edec11ce86a1a0d9e4c5a2a0df6acaf655041c24/django/db/backends/base/schema.py#L97
the schema editor can rollback DDL] but the current `sqlmigrate` logic
only takes `migration.atomic` into consideration.
The issue can be addressed by
1. Changing `sqlmigrate`
[https://github.com/django/django/blob/edec11ce86a1a0d9e4c5a2a0df6acaf655041c24/django/core/management/commands/sqlmigrate.py#L59
assignment] of `self.output_transaction` to consider
`connection.features.can_rollback_ddl` as well.
2. Adding a test in `tests/migrations/test_commands.py` based on
[https://github.com/django/django/blob/2bd8df243ac6fc35e58c9fe90b20c9e42519a5ac/tests/migrations/test_commands.py#L606-L617
an existing test for non-atomic migrations] that mocks
`connection.features.can_rollback_ddl` to `False` instead of overdidding
`MIGRATION_MODULES` to point to a non-atomic migration.
I marked the ticket as ''easy picking'' because I included the above
guidelines but feel free to uncheck it if you deem it inappropriate.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/30189#comment:1>
Django
support transactional DDL
Reporter: Simon Charette | Owner: Parth Patil
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Parth Patil):
* owner: nobody => Parth Patil
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/30189#comment:2>
Django
support transactional DDL
Reporter: Simon Charette | Owner: Parth Patil
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Unreviewed => Accepted
Comment:
> I marked the ticket as easy picking because I included the above
guidelines but feel free to uncheck it if you deem it inappropriate.
Super. We don't have enough Easy Pickings tickets for the demand, so this
kind of thing is great. (IMO 🙂)
--
Ticket URL: <https://code.djangoproject.com/ticket/30189#comment:3>
Django
support transactional DDL
Reporter: Simon Charette | Owner: Parth Patil
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Parth Patil):
Hey, I'm working on this ticket, I would like you to know as this is my
first ticket it may take little longer to complete :).
Here is a [https://github.com/Parth1811/django/tree/ticket_30189 | link]
to the working branch
You may feel free to post references or elaborate more on the topic.
--
Ticket URL: <https://code.djangoproject.com/ticket/30189#comment:4>
Django
support transactional DDL
Reporter: Simon Charette | Owner: Parth Patil
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Carlton Gibson):
Hi Parth. No problem. If you need help please reach out to e.g.
[https://groups.google.com/forum/#!forum/django-core-mentorship django-
core-mentorship] citing this issue, and where you've got to/got stuck.
Welcome aboard, and have fun! ✨
--
Ticket URL: <https://code.djangoproject.com/ticket/30189#comment:5>
Django
support transactional DDL
Reporter: Simon Charette | Owner: Parth Patil
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Parth Patil):
* needs_docs: 0 => 1
* has_patch: 0 => 1
Comment:
here is a link to the patch
[https://github.com/django/django/pull/11039|https://github.com/django/django/pull/11039]
--
Ticket URL: <https://code.djangoproject.com/ticket/30189#comment:6>
Django
support transactional DDL
Reporter: Simon Charette | Owner: Parth Patil
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 1 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"acc041984fac1e79d9a1258c94479dd3ffc7f186" acc04198]:
{{{
#!CommitTicketReference repository=""
revision="acc041984fac1e79d9a1258c94479dd3ffc7f186"
Fixed #30189 -- Removed transaction from sqlmigrate output if database
doesn't use one.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30189#comment:7>