[Django] #27928: Gratuitous ALTER COLUMN statements generated for adding blank=True
Django
---------------------------------------------+------------------------
Reporter: Christophe Pettus | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
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 |
---------------------------------------------+------------------------
The migration framework, when changing blank=False to blank=True on a
statement with null=False, generates gratuitous `ALTER COLUMN ... ADD
DEFAULT ''` and `ALTER COLUMN DROP DEFAULT` SQL statements. Example:
{{{
class Item(models.Model):
id = models.UUIDField(primary_key=True, default=uuid.uuid4,
editable=False)
title = models.CharField(max_length=50, blank=True) # blank=True
added where not previously present
}}}
generates, from `sqlmigration`:
{{{
BEGIN;
--
-- Alter field title on item
--
ALTER TABLE "news_item" ALTER COLUMN "title" SET DEFAULT '';
ALTER TABLE "news_item" ALTER COLUMN "title" DROP DEFAULT;
COMMIT;
}}}
Besides being slightly messy, this can create unexpected locking behavior.
--
Ticket URL: <https://code.djangoproject.com/ticket/27928>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Christophe Pettus | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
Comment:
Looks like an issue with
[https://github.com/django/django/blob/75503a823f6358892fff5aeb3691683ad9cc5a60/django/db/backends/base/schema.py#L191-L217
SchemaEditor.effective_default()]. It returns `None` for the old field and
`''` (empty string) for the new field, thus the
[https://github.com/django/django/blob/75503a823f6358892fff5aeb3691683ad9cc5a60/django/db/backends/base/schema.py#L578-L584
needs_database_default] condition is True. Probably `effective_default()`
should return an empty string for the old field.
--
Ticket URL: <https://code.djangoproject.com/ticket/27928#comment:1>
Django
Reporter: Christophe Pettus | Owner:
| Christophe Pettus
Type: Bug | Status: assigned
Component: Migrations | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Christophe Pettus):
* owner: nobody => Christophe Pettus
* status: new => assigned
Comment:
It's superficially an easy fix. The problem stems from `blank=True` being
overloaded to mean "can be blank in forms" and "is blank by default."
--
Ticket URL: <https://code.djangoproject.com/ticket/27928#comment:2>
Django
Reporter: Christophe Pettus | Owner:
| Christophe Pettus
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Christophe Pettus):
Patch is ready to go, and existing test suite passes. I'm not 100% sure
how best to approach a regression test for this; guidance welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/27928#comment:3>
Django
Reporter: Christophe Pettus | Owner:
| Christophe Pettus
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
The place to put the test is probably `tests/schema/tests.py`. Can you use
`assertNumQueries()` to check that no queries happen for the
`alter_field()` call?
--
Ticket URL: <https://code.djangoproject.com/ticket/27928#comment:4>
Django
Reporter: Christophe Pettus | Owner:
| Christophe Pettus
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Can you share the patch you wrote? The patch for #28000 might fix this.
--
Ticket URL: <https://code.djangoproject.com/ticket/27928#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Christophe Pettus | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* owner: Christophe Pettus => Simon Charette
* version: 1.10 => master
Comment:
I'll reuse #28000's summary which was closed as duplicate as it exemplify
the general issue more appropriately.
In the subcase reported here setting `blank=True` changes the effective
default for operations adding/altering non-nullable fields which trigger
the actual bug.
--
Ticket URL: <https://code.djangoproject.com/ticket/27928#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Christophe Pettus | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/8278 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/27928#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Christophe Pettus | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/27928#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Christophe Pettus | Owner: Simon
| Charette
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette <charette.s@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"35c0025151ad411e691a2fa62a0dd3507ebafd82" 35c0025]:
{{{
#!CommitTicketReference repository=""
revision="35c0025151ad411e691a2fa62a0dd3507ebafd82"
Fixed #27928 -- Avoided SET/DROP DEFAULT unless a field changes from null
to non-null.
Thanks Christophe Pettus, Matteo Pietro Russo for reports and Tim for
review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27928#comment:9>