Opened 7 years ago

Closed 7 years ago

#27928 closed Bug (fixed)

Avoid SET/DROP DEFAULT unless a field changes from null to non-null

Reported by: Christophe Pettus Owned by: Simon Charette
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (9)

comment:1 by Tim Graham, 7 years ago

Triage Stage: UnreviewedAccepted

Looks like an issue with SchemaEditor.effective_default(). It returns None for the old field and '' (empty string) for the new field, thus the needs_database_default condition is True. Probably effective_default() should return an empty string for the old field.

comment:2 by Christophe Pettus, 7 years ago

Owner: changed from nobody to Christophe Pettus
Status: newassigned

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."

comment:3 by Christophe Pettus, 7 years ago

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.

comment:4 by Tim Graham, 7 years ago

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?

comment:5 by Tim Graham, 7 years ago

Can you share the patch you wrote? The patch for #28000 might fix this.

comment:6 by Simon Charette, 7 years ago

Owner: changed from Christophe Pettus to Simon Charette
Summary: Gratuitous ALTER COLUMN statements generated for adding blank=TrueAvoid SET/DROP DEFAULT unless a field changes from null to non-null
Version: 1.10master

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.

comment:7 by Simon Charette, 7 years ago

Has patch: set

comment:8 by Tim Graham, 7 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Simon Charette <charette.s@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In 35c0025:

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.

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