Skip to content

Commit 3e10ab2

Browse files
committed
Move column nullability and DEFAULT ownership to convergence
Removes the schema editor's 4-way backfill for nullable -> NOT NULL transitions and delegates nullability + column DEFAULT reconciliation entirely to convergence, which already handles them with online-safe DDL (CHECK NOT VALID -> VALIDATE -> SET NOT NULL; catalog-only SET/DROP DEFAULT). - Add allow_null to the previously named non_db_attrs so the schema editor short-circuits nullability-only alters, same pattern that already covered default=. - Rename non_db_attrs -> non_migration_attrs. Post-refactor the attribute covers allow_null, default, on_delete — all real DB concepts, just not owned by migrations. "non_db" was a Django-ism that no longer matches Plain's architecture. - Strip non_migration_attrs in the autodetector before deciding to emit AlterField, so migration files no longer carry no-op operations for nullability/default/on_delete/choices-only diffs. These were previously generated and silently short-circuited at apply time. - Delete the autodetector's "alter nullable -> NOT NULL without default" MigrationSchemaError. Convergence reports NullabilityDrift with has_null_rows=True as a blocked fix and tells the user to backfill; no longer a structural failure. - Delete unused schema editor helpers (_alter_column_null_sql, _alter_column_default_sql), SQL templates (sql_alter_column_null / _not_null / _default, sql_update_with_default / _db_default), and the dead sql_alter_column_type class attr. Clean up the unused new_type param on _rename_field_sql. - Simplify _alter_column_type_sql: drops the always-[] post_actions tuple and the model / old_field parameters it never used; uses a local variable instead of mutating self.sql_alter_column_type on each call. - README: add "How do I make an existing column NOT NULL?" FAQ. Update the add-a-field FAQ to point at the convergence-driven flow (add allow_null=True, data-migrate, remove allow_null=True). Drop the 4-way backfill example from the DDL-timeouts paragraph. Upgrade path: projects upgrading will keep any historical AlterField operations from past allow_null flips. These re-apply via the schema editor, which now no-ops on those diffs, so migrate apply is safe. The first postgres sync after upgrade will surface NullabilityDrift for any column whose DB state disagrees with the model — intended behavior; convergence applies SetNotNullFix automatically if no NULL rows exist, or blocks with guidance if they do.
1 parent 07cb500 commit 3e10ab2

9 files changed

Lines changed: 155 additions & 474 deletions

File tree

plain-postgres/plain/postgres/README.md

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -693,7 +693,7 @@ POSTGRES_CONVERGENCE_STATEMENT_TIMEOUT = "3s"
693693

694694
`lock_timeout` applies to every DDL. `statement_timeout` applies only to statements that take `ACCESS EXCLUSIVE` — non-blocking operations (`CREATE INDEX CONCURRENTLY`, `VALIDATE CONSTRAINT`) run unbounded because they can't cascade the lock queue.
695695

696-
If a migration issues a row-touching UPDATE (e.g. the 4-way backfill that runs when you flip a nullable column to `NOT NULL` with a persistent default), the 3s `statement_timeout` will kill it on any non-tiny table. That's intentional — the right fix is a batched data migration, not a hidden inline backfill. The common first-time failure mode is applying migrations against a pre-seeded dev or staging database: raise the ceiling for that one run, then lower it back for production deploys.
696+
If a migration issues a row-touching UPDATE (e.g. a hand-written `RunPython` or `RunSQL` backfill), the 3s `statement_timeout` will kill it on any non-tiny table. That's intentional — the right fix is a batched data migration, not a single long-running UPDATE. The common first-time failure mode is applying migrations against a pre-seeded dev or staging database: raise the ceiling for that one run, then lower it back for production deploys.
697697

698698
Use `RunSQL(no_timeout=True)` to opt out for a specific operation:
699699

@@ -1222,7 +1222,14 @@ Add the field to your model class, then run `plain migrations create` to create
12221222
If the field is required (no `default=` and not `allow_null=True`), the autodetector refuses to generate the migration, since there's no value to seed existing rows with. You have two options:
12231223

12241224
1. Declare a `default=` on the field so the new column has a value for existing rows.
1225-
2. Add the field as nullable first, scaffold a data migration with `plain migrations create --empty --name backfill_<field>` to populate existing rows, then alter the field to NOT NULL in a third migration.
1225+
2. Add the field with `allow_null=True`, scaffold a data migration with `plain migrations create --empty --name backfill_<field>` to populate existing rows, then remove `allow_null=True` from the field — convergence applies `NOT NULL` on the next `postgres sync`.
1226+
1227+
#### How do I make an existing column `NOT NULL`?
1228+
1229+
Edit the field to remove `allow_null=True`. `plain migrations create` won't detect a schema change — nullability is managed by convergence. Run `plain postgres sync`:
1230+
1231+
- If the column has no `NULL` rows, convergence applies the change with a non-blocking `CHECK NOT VALID` + `VALIDATE` + `SET NOT NULL` pattern.
1232+
- If `NULL` rows exist, convergence blocks and prints the table and column to backfill. Scaffold a data migration with `plain migrations create --empty --name backfill_<field>`, write the backfill, and run `postgres sync` again.
12261233

12271234
#### How do I create a unique constraint on multiple fields?
12281235

plain-postgres/plain/postgres/fields/base.py

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ class Field[T](RegisterLookupMixin):
132132
# Kwargs that don't affect the column definition; the schema editor
133133
# ignores these when deciding whether an ALTER is needed. Subclasses
134134
# that introduce additional non-db kwargs extend this tuple.
135-
non_db_attrs: tuple[str, ...] = ()
135+
non_migration_attrs: tuple[str, ...] = ()
136136

137137
def __init__(self) -> None:
138138
self.name = None # Set by set_attributes_from_name
@@ -536,7 +536,12 @@ def value_from_object(self, obj: Model) -> T | None:
536536
class ColumnField[T](Field[T]):
537537
"""Base for fields backed by a column value (required/allow_null/validators)."""
538538

539-
non_db_attrs = (*Field.non_db_attrs, "required", "validators")
539+
non_migration_attrs = (
540+
*Field.non_migration_attrs,
541+
"required",
542+
"validators",
543+
"allow_null",
544+
)
540545

541546
def __init__(
542547
self,
@@ -642,7 +647,7 @@ def deconstruct(self) -> tuple[str | None, str, list[Any], dict[str, Any]]:
642647
class DefaultableField[T](ColumnField[T]):
643648
"""Base for column-backed fields that accept a user-provided ``default``."""
644649

645-
non_db_attrs = (*ColumnField.non_db_attrs, "default")
650+
non_migration_attrs = (*ColumnField.non_migration_attrs, "default")
646651

647652
def __init__(
648653
self,
@@ -700,7 +705,7 @@ def deconstruct(self) -> tuple[str | None, str, list[Any], dict[str, Any]]:
700705
class ChoicesField[T](DefaultableField[T]):
701706
"""Base for fields that accept a ``choices=`` parameter."""
702707

703-
non_db_attrs = (*DefaultableField.non_db_attrs, "choices")
708+
non_migration_attrs = (*DefaultableField.non_migration_attrs, "choices")
704709

705710
def __init__(
706711
self,

plain-postgres/plain/postgres/fields/related.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,11 @@ def lazy_related_operation(
9696
class RelatedField(FieldCacheMixin, Field):
9797
"""Base class that all relational fields inherit from."""
9898

99-
non_db_attrs = (*Field.non_db_attrs, "limit_choices_to", "related_query_name")
99+
non_migration_attrs = (
100+
*Field.non_migration_attrs,
101+
"limit_choices_to",
102+
"related_query_name",
103+
)
100104

101105
# RelatedField always has a remote_field (never None)
102106
remote_field: ForeignObjectRel
@@ -359,7 +363,11 @@ class ForeignKeyField(ColumnField, RelatedField):
359363
ForeignKeyField targets the primary key (id) of the remote model.
360364
"""
361365

362-
non_db_attrs = (*RelatedField.non_db_attrs, *ColumnField.non_db_attrs, "on_delete")
366+
non_migration_attrs = (
367+
*RelatedField.non_migration_attrs,
368+
*ColumnField.non_migration_attrs,
369+
"on_delete",
370+
)
363371

364372
empty_strings_allowed = False
365373

plain-postgres/plain/postgres/migrations/autodetector.py

Lines changed: 24 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,17 @@
2727
from plain.postgres.migrations.state import ProjectState
2828

2929

30+
def _strip_non_migration_attrs(
31+
dec: tuple[str, list[Any], dict[str, Any]], non_migration_attrs: tuple[str, ...]
32+
) -> tuple[str, list[Any], dict[str, Any]]:
33+
# Mirrors DatabaseSchemaEditor._field_should_be_altered — the schema editor
34+
# already short-circuits when only non_migration_attrs differ; stripping
35+
# them here skips the no-op AlterField too, so migration files reflect
36+
# real DDL.
37+
path, args, kwargs = dec
38+
return path, args, {k: v for k, v in kwargs.items() if k not in non_migration_attrs}
39+
40+
3041
class MigrationAutodetector:
3142
"""
3243
Take a pair of ProjectStates and compare them to see what the first would
@@ -804,9 +815,9 @@ def _generate_added_field(
804815
f"Choose one:\n"
805816
f" 1. Declare a default on the field in models.py, e.g.\n"
806817
f' {field_name}: str = types.TextField(default="...")\n'
807-
f" 2. Split into two migrations: add the field as nullable, "
808-
f"write a data migration to populate existing rows, then alter "
809-
f"to NOT NULL:\n"
818+
f" 2. Add the field with allow_null=True, scaffold a data "
819+
f"migration to populate existing rows, then remove allow_null=True "
820+
f"— convergence applies NOT NULL on the next sync:\n"
810821
f" uv run plain migrations create --empty --name backfill_{field_name}"
811822
)
812823
self.add_operation(
@@ -898,6 +909,16 @@ def generate_altered_fields(self) -> None:
898909
new_field.remote_field.through = old_field.remote_field.through # ty: ignore[unresolved-attribute]
899910
old_field_dec = self.deep_deconstruct(old_field)
900911
new_field_dec = self.deep_deconstruct(new_field)
912+
# Exclude non_migration_attrs (allow_null, default, on_delete, choices,
913+
# etc.) — convergence handles those transitions, and the schema
914+
# editor short-circuits when only non_migration_attrs differ. Emitting
915+
# an AlterField here would be a no-op at apply time.
916+
old_field_dec = _strip_non_migration_attrs(
917+
old_field_dec, old_field.non_migration_attrs
918+
)
919+
new_field_dec = _strip_non_migration_attrs(
920+
new_field_dec, new_field.non_migration_attrs
921+
)
901922
if old_field_dec != new_field_dec and old_field_name == field_name:
902923
both_m2m = isinstance(old_field, ManyToManyField) and isinstance(
903924
new_field, ManyToManyField
@@ -907,26 +928,6 @@ def generate_altered_fields(self) -> None:
907928
) and not isinstance(new_field, ManyToManyField)
908929
if both_m2m or neither_m2m:
909930
# Either both fields are m2m or neither is
910-
if (
911-
isinstance(old_field, ColumnField)
912-
and isinstance(new_field, ColumnField)
913-
and old_field.allow_null
914-
and not new_field.allow_null
915-
and not new_field.has_persistent_column_default()
916-
):
917-
raise MigrationSchemaError(
918-
f"Cannot alter field '{model_name}.{field_name}' "
919-
f"from nullable to NOT NULL without a default. "
920-
f"Existing NULL rows have no value to fall back "
921-
f"on.\n\n"
922-
f"Choose one:\n"
923-
f" 1. Declare a default on the field in models.py, e.g.\n"
924-
f' {field_name}: str = types.TextField(default="...")\n'
925-
f" 2. Split into two migrations: write a data "
926-
f"migration to populate the NULL rows first, then "
927-
f"alter to NOT NULL:\n"
928-
f" uv run plain migrations create --empty --name backfill_{field_name}"
929-
)
930931
self.add_operation(
931932
package_label,
932933
operations.AlterField(

0 commit comments

Comments
 (0)