From 963494763879f735a64209304b3688d35bf63fb2 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Tue, 14 Jul 2026 10:11:37 -0700 Subject: [PATCH 1/3] Mark deprecated lanes.size columns as deferred (PP-4506) The plan to drop lanes.size / size_by_entrypoint directly fails the backwards-compatibility gate: the previous release still maps those columns, so its ORM SELECTs them, and dropping the columns makes those SELECTs error ("column lanes.size does not exist"). Because SQLAlchemy eagerly SELECTs every mapped column, "keeping them mapped" (as the prior release did) is not enough to stop using them. Mark them ``deferred`` instead: they stay in the model and the schema (so test_model_definitions_match_ddl still passes and the previous release keeps working), but the current code no longer includes them in its default SELECT. This is the online-migration "stop using the column" step. Once a release containing this change has shipped, a follow-up migration can drop the columns without breaking the then-previous release. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/palace/manager/sqlalchemy/model/lane.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/palace/manager/sqlalchemy/model/lane.py b/src/palace/manager/sqlalchemy/model/lane.py index cab6284e25..242ad9b432 100644 --- a/src/palace/manager/sqlalchemy/model/lane.py +++ b/src/palace/manager/sqlalchemy/model/lane.py @@ -18,6 +18,7 @@ from sqlalchemy.ext.associationproxy import association_proxy from sqlalchemy.orm import ( Mapped, + deferred, relationship, ) from sqlalchemy.orm.session import Session @@ -112,11 +113,13 @@ class Lane(Base, DatabaseBackedWorkList, HierarchyWorkList): priority: Mapped[int] = Column(Integer, index=True, nullable=False, default=0) # Deprecated: these cached size estimates are no longer populated or read. - # The code that maintained them (update_size and the lane-size Celery tasks) - # has been removed; the columns remain mapped only so the model continues to - # match the database schema, and will be dropped in a follow-up migration. - size: Mapped[int] = Column(Integer, nullable=False, default=0) - size_by_entrypoint = Column(JSON, nullable=True) + # They are mapped as ``deferred`` so the ORM does not include them in its + # default SELECT (nothing accesses them). This is the online-migration + # "stop using the column" step: once this release ships, a follow-up + # migration can drop the columns without breaking the still-running previous + # release, which would otherwise SELECT them and fail. + size = deferred(Column(Integer, nullable=False, default=0)) + size_by_entrypoint = deferred(Column(JSON, nullable=True)) # A lane may have one parent lane and many sublanes. sublanes: Mapped[list[Lane]] = relationship( From 7f06dd479c599c85bb8135e61deed6f80ee0b5a5 Mon Sep 17 00:00:00 2001 From: Jonathan Green Date: Wed, 15 Jul 2026 10:46:46 -0300 Subject: [PATCH 2/3] Make lanes.size write-safe via server_default before drop (PP-4506) Marking the columns ``deferred`` stops reads (the default SELECT) but not writes: ``size`` is NOT NULL with a Python-side ``default=0``, so SQLAlchemy still emitted it in every Lane INSERT (lanes are created at runtime via create_default_lanes). A follow-up migration that dropped the column would then break the still-running previous release's inserts. Move the default into the database with an ``ALTER COLUMN size SET DEFAULT 0`` migration and drop the Python-side ``default=0``. With a server_default and no Python default, SQLAlchemy omits the column from INSERTs and Postgres fills in 0, so the eventual DROP is write-safe. The migration is catalog-only (instant, no rewrite) and backwards-compatible: N-1 still writes size=0 explicitly. ``size_by_entrypoint`` is nullable with no default, so it was already omitted. Also document in CLAUDE.md that "stop using" an ORM column means stopping both reads (deferred) and writes (server_default), so this trap is avoided going forward. --- CLAUDE.md | 21 ++++++++++++ ...224c1b_add_server_default_to_lanes_size.py | 33 +++++++++++++++++++ src/palace/manager/sqlalchemy/model/lane.py | 19 +++++++---- 3 files changed, 67 insertions(+), 6 deletions(-) create mode 100644 alembic/versions/20260715_dbeb42224c1b_add_server_default_to_lanes_size.py diff --git a/CLAUDE.md b/CLAUDE.md index 7a23dc7345..4566353c43 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -149,6 +149,27 @@ code stops using it. Removing a schema object is therefore a two-step process ac 1. **Release 1:** Stop using the object in the code, but leave it in the schema. No drop migration yet. 2. **Release 2:** Drop the object in a migration, now that no running code (current or N-1) references it. +**"Stop using" an ORM column means stopping both reads _and_ writes.** SQLAlchemy eagerly includes every +mapped column in its default `SELECT`, and eagerly emits any column that has a Python-side `default=` in every +`INSERT`. So leaving a column mapped — even when no application code reads or assigns it — does **not** stop +using it, and a later drop will break the still-running previous release. `test_model_definitions_match_ddl` +passing does not catch this; it only checks that the model matches the schema, not that inserts avoid the +column. To fully stop using a column in release 1, before dropping it in release 2: + +- **Reads:** mark the column `deferred()` so it is excluded from the default `SELECT`. +- **Writes:** remove any Python-side `default=`. If the column is `NOT NULL`, removing the Python default + alone makes inserts fail the not-null constraint (SQLAlchemy sends no value and the database has none), so + first move the default into the database with a `server_default` via an `ALTER COLUMN ... SET DEFAULT` + migration — catalog-only in Postgres (instant, no rewrite) and backwards-compatible because N-1 still writes + the value explicitly, harmlessly overriding the default. With a `server_default` set and the Python + `default=` removed, SQLAlchemy omits the column from `INSERT`s and the database fills it in. A **nullable** + column with no default is already omitted from `INSERT`s, so it needs no write-side change. + +Both the read-side (`deferred`) and write-side (`server_default`) changes are backwards-compatible, so they +belong together in **release 1**; the drop is **release 2**. There is no ORM-only way to make the drop +write-safe without this schema change: any attempt to stop emitting a `NOT NULL`/no-default column either +keeps emitting it (drop still breaks) or fails inserts immediately. + The same constraint applies in reverse when adding required schema: a new non-nullable column must first be added as nullable, or with a **server default** so the database fills it in for rows written by N-1 code (which does not yet know about the column). Never write a single migration that both adds a not-yet-used column as diff --git a/alembic/versions/20260715_dbeb42224c1b_add_server_default_to_lanes_size.py b/alembic/versions/20260715_dbeb42224c1b_add_server_default_to_lanes_size.py new file mode 100644 index 0000000000..41d5c4aeb5 --- /dev/null +++ b/alembic/versions/20260715_dbeb42224c1b_add_server_default_to_lanes_size.py @@ -0,0 +1,33 @@ +"""Add server_default to lanes.size + +Revision ID: dbeb42224c1b +Revises: a6c85605404c +Create Date: 2026-07-15 13:42:28.828886+00:00 + +""" + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "dbeb42224c1b" +down_revision = "a6c85605404c" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + # ``lanes.size`` is deprecated and no longer read or written by the current + # code, but it is NOT NULL with no database default. Moving the default into + # the database lets the ORM omit the column from INSERTs (it previously + # supplied a Python-side ``default=0``), so a follow-up release can drop the + # column without breaking the still-running previous release's inserts. + # + # ``SET DEFAULT`` is a catalog-only change in Postgres: instant, no table + # rewrite. It is backwards-compatible because the previous release still + # explicitly writes ``size=0``, which simply overrides the new default. + op.alter_column("lanes", "size", server_default=sa.text("0")) + + +def downgrade() -> None: + op.alter_column("lanes", "size", server_default=None) diff --git a/src/palace/manager/sqlalchemy/model/lane.py b/src/palace/manager/sqlalchemy/model/lane.py index 242ad9b432..e6b4e21f60 100644 --- a/src/palace/manager/sqlalchemy/model/lane.py +++ b/src/palace/manager/sqlalchemy/model/lane.py @@ -13,6 +13,7 @@ Unicode, UniqueConstraint, or_, + text, ) from sqlalchemy.dialects.postgresql import ARRAY, INT4RANGE, JSON from sqlalchemy.ext.associationproxy import association_proxy @@ -113,12 +114,18 @@ class Lane(Base, DatabaseBackedWorkList, HierarchyWorkList): priority: Mapped[int] = Column(Integer, index=True, nullable=False, default=0) # Deprecated: these cached size estimates are no longer populated or read. - # They are mapped as ``deferred`` so the ORM does not include them in its - # default SELECT (nothing accesses them). This is the online-migration - # "stop using the column" step: once this release ships, a follow-up - # migration can drop the columns without breaking the still-running previous - # release, which would otherwise SELECT them and fail. - size = deferred(Column(Integer, nullable=False, default=0)) + # They are mapped as ``deferred`` so the ORM excludes them from its default + # SELECT (the read side), and this release must also stop writing them so a + # follow-up migration can drop the columns without breaking the still-running + # previous release. + # + # ``size`` is NOT NULL, so removing the Python-side ``default=0`` alone would + # make inserts fail the not-null constraint. Instead we move the default into + # the database as a ``server_default``: with no Python default set, SQLAlchemy + # omits the column from INSERTs entirely and Postgres fills in 0. That is what + # makes the eventual DROP write-safe. ``size_by_entrypoint`` is nullable with + # no default, so SQLAlchemy already omits it from INSERTs. + size = deferred(Column(Integer, nullable=False, server_default=text("0"))) size_by_entrypoint = deferred(Column(JSON, nullable=True)) # A lane may have one parent lane and many sublanes. From 13b75b6e40563646ba694a5bf48a77a8adce6a13 Mon Sep 17 00:00:00 2001 From: Jonathan Green Date: Wed, 15 Jul 2026 10:58:39 -0300 Subject: [PATCH 3/3] Some small human comment cleanup --- CLAUDE.md | 16 +++++----------- ...b42224c1b_add_server_default_to_lanes_size.py | 4 ---- src/palace/manager/sqlalchemy/model/lane.py | 15 ++++----------- 3 files changed, 9 insertions(+), 26 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4566353c43..1a8d6739d8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -152,23 +152,17 @@ code stops using it. Removing a schema object is therefore a two-step process ac **"Stop using" an ORM column means stopping both reads _and_ writes.** SQLAlchemy eagerly includes every mapped column in its default `SELECT`, and eagerly emits any column that has a Python-side `default=` in every `INSERT`. So leaving a column mapped — even when no application code reads or assigns it — does **not** stop -using it, and a later drop will break the still-running previous release. `test_model_definitions_match_ddl` -passing does not catch this; it only checks that the model matches the schema, not that inserts avoid the -column. To fully stop using a column in release 1, before dropping it in release 2: +using it, and a later drop will break the still-running previous release. To fully stop using a column in +release 1, before dropping it in release 2: - **Reads:** mark the column `deferred()` so it is excluded from the default `SELECT`. - **Writes:** remove any Python-side `default=`. If the column is `NOT NULL`, removing the Python default alone makes inserts fail the not-null constraint (SQLAlchemy sends no value and the database has none), so - first move the default into the database with a `server_default` via an `ALTER COLUMN ... SET DEFAULT` - migration — catalog-only in Postgres (instant, no rewrite) and backwards-compatible because N-1 still writes - the value explicitly, harmlessly overriding the default. With a `server_default` set and the Python - `default=` removed, SQLAlchemy omits the column from `INSERT`s and the database fills it in. A **nullable** - column with no default is already omitted from `INSERT`s, so it needs no write-side change. + first move the default into the database with a `server_default`. A **nullable** column with no default + is already omitted from `INSERT`s, so it needs no write-side change. Both the read-side (`deferred`) and write-side (`server_default`) changes are backwards-compatible, so they -belong together in **release 1**; the drop is **release 2**. There is no ORM-only way to make the drop -write-safe without this schema change: any attempt to stop emitting a `NOT NULL`/no-default column either -keeps emitting it (drop still breaks) or fails inserts immediately. +belong together in **release 1**; the drop is **release 2**. The same constraint applies in reverse when adding required schema: a new non-nullable column must first be added as nullable, or with a **server default** so the database fills it in for rows written by N-1 code (which diff --git a/alembic/versions/20260715_dbeb42224c1b_add_server_default_to_lanes_size.py b/alembic/versions/20260715_dbeb42224c1b_add_server_default_to_lanes_size.py index 41d5c4aeb5..2efbb323b9 100644 --- a/alembic/versions/20260715_dbeb42224c1b_add_server_default_to_lanes_size.py +++ b/alembic/versions/20260715_dbeb42224c1b_add_server_default_to_lanes_size.py @@ -22,10 +22,6 @@ def upgrade() -> None: # the database lets the ORM omit the column from INSERTs (it previously # supplied a Python-side ``default=0``), so a follow-up release can drop the # column without breaking the still-running previous release's inserts. - # - # ``SET DEFAULT`` is a catalog-only change in Postgres: instant, no table - # rewrite. It is backwards-compatible because the previous release still - # explicitly writes ``size=0``, which simply overrides the new default. op.alter_column("lanes", "size", server_default=sa.text("0")) diff --git a/src/palace/manager/sqlalchemy/model/lane.py b/src/palace/manager/sqlalchemy/model/lane.py index e6b4e21f60..b090a6c431 100644 --- a/src/palace/manager/sqlalchemy/model/lane.py +++ b/src/palace/manager/sqlalchemy/model/lane.py @@ -114,17 +114,10 @@ class Lane(Base, DatabaseBackedWorkList, HierarchyWorkList): priority: Mapped[int] = Column(Integer, index=True, nullable=False, default=0) # Deprecated: these cached size estimates are no longer populated or read. - # They are mapped as ``deferred`` so the ORM excludes them from its default - # SELECT (the read side), and this release must also stop writing them so a - # follow-up migration can drop the columns without breaking the still-running - # previous release. - # - # ``size`` is NOT NULL, so removing the Python-side ``default=0`` alone would - # make inserts fail the not-null constraint. Instead we move the default into - # the database as a ``server_default``: with no Python default set, SQLAlchemy - # omits the column from INSERTs entirely and Postgres fills in 0. That is what - # makes the eventual DROP write-safe. ``size_by_entrypoint`` is nullable with - # no default, so SQLAlchemy already omits it from INSERTs. + # The code that maintained them (update_size and the lane-size Celery tasks) + # has been removed; the columns remain mapped only so the model continues to + # match the database schema, and will be dropped in a follow-up migration. + # TODO: Drop these next release when it is safe to do so. size = deferred(Column(Integer, nullable=False, server_default=text("0"))) size_by_entrypoint = deferred(Column(JSON, nullable=True))