Skip to content

Drop deprecated lanes.size columns (PP-4506)#3563

Draft
jonathangreen wants to merge 1 commit into
mainfrom
chore/drop-lane-size-columns
Draft

Drop deprecated lanes.size columns (PP-4506)#3563
jonathangreen wants to merge 1 commit into
mainfrom
chore/drop-lane-size-columns

Conversation

@jonathangreen

@jonathangreen jonathangreen commented Jul 15, 2026

Copy link
Copy Markdown
Member

Description

Release-2 (the drop) of the online-migration sequence for removing the deprecated lane-size cache columns. The release-1 PR (#3481) has merged: it marked size / size_by_entrypoint as deferred() (stop reading) and moved size's default into the database via server_default (stop writing, so the column is omitted from INSERTs). This PR removes the columns from the Lane model and drops them from the schema.

I created this so we don't forget to drop these columns since I merged #3481 and @dbernstein is out on vacation.

⚠️ Do not merge until #3481 has been deployed

This is safe only once a release containing #3481 has actually gone out. I'll leave this in draft until then.

Motivation and Context

JIRA: PP-4506

Completes the two-release removal started in #3481.

How Has This Been Tested?

  • tests/migration/test_alembic_builtin_tests.py passes, including test_model_definitions_match_ddl (model matches the schema after the drop) and the up/down migration round-trip (downgrade recreates the columns, upgrade drops them again).
  • mypy clean on the Lane model.
  • Confirmed no remaining references to size / size_by_entrypoint on Lane anywhere in src/ or tests/.

Checklist

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

Release-2 of the online-migration sequence started in the parent PR. Now that a
release marking size / size_by_entrypoint as deferred and moving size's default
into the database (server_default) has shipped, no running release (current or
N-1) reads or writes these columns, so they can be dropped safely.

Removes the columns from the Lane model (and the now-unused deferred / text /
JSON imports) and adds the drop migration. The downgrade recreates the columns
in their post-release-1 state (size NOT NULL with server_default 0,
size_by_entrypoint nullable JSON).

Stacked on the release-1 PR; do not merge until that release has gone out.
@jonathangreen jonathangreen added the DB migration This PR contains a DB migration label Jul 15, 2026
@claude

claude Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jonathangreen's task in 2m 28s —— View job


Review

  • Gather context (diff, model, migration chain, prior release-1 PR)
  • Verify migration correctness (drop/downgrade round-trip, revision chain)
  • Verify model changes & no lingering references
  • Post review

Summary

This is a clean, correct release-2 drop and the online-migration reasoning holds up. I traced the full sequence: release-1 (dbeb42224c1b) added server_default 0 to size and #3481 marked both columns deferred(), so N-1 code neither SELECTs them (deferred → excluded from the default select) nor sends them on INSERT (no Python-side default= → column omitted). Dropping them therefore doesn't break the still-running previous release. The migration chains linearly off the current head (dbeb42224c1b is a single head; nothing references de6ae4bbf4a5), the downgrade faithfully recreates the post-release-1 state (size NOT NULL / server_default 0, size_by_entrypoint nullable JSON with the standard astext_type=sa.Text()), and there are no leftover autogenerated alembic comments. On the model side, the three removed imports (text, deferred, JSON) are confirmed unused elsewhere in lane.py, and I confirmed no remaining references to Lane.size / size_by_entrypoint anywhere in src/ or tests/ (the .size, update_size, and featured_lane_size hits all belong to CustomList, Pagination, image.size, or the unrelated library setting). The one thing that matters here is procedural rather than in the diff — as your PR body already states in bold, this must not merge until a release containing #3481 has actually shipped — and you've handled that correctly. No actionable findings.

@greptile-apps

greptile-apps Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This is the second (drop) release of a two-step online migration that removes the deprecated size and size_by_entrypoint columns from the lanes table. Release 1 (#3481) already marked both columns deferred() and moved size's default to the database, so no running release reads or writes them.

  • Model change: Removes the two deprecated Lane column mappings and cleans up the now-unused imports (deferred, text, postgresql JSON).
  • Migration: upgrade() drops both columns in the correct release-2 window; downgrade() faithfully recreates them in their post-release-1 state (size NOT NULL with server_default=0, size_by_entrypoint nullable JSON).

Confidence Score: 5/5

Safe to merge once the release containing #3481 has been deployed; the migration and model change are self-consistent and complete.

The two-release online-migration pattern is followed correctly: release 1 stopped all reads and writes on both columns, and this release drops them. The downgrade recreates both columns in exactly their post-release-1 state. No remaining references to the dropped columns exist anywhere in src/ or tests/. The model cleanup (removed imports, removed mappings) is consistent with the migration.

No files require special attention. The migration and model change are straightforward and fully consistent with each other.

Important Files Changed

Filename Overview
alembic/versions/20260715_de6ae4bbf4a5_drop_deprecated_lanes_size_columns.py New migration: drops size_by_entrypoint then size from lanes; downgrade correctly recreates both columns in their post-release-1 state (size NOT NULL with server_default=0, size_by_entrypoint nullable JSON).
src/palace/manager/sqlalchemy/model/lane.py Removes the two deprecated deferred column mappings (size, size_by_entrypoint) and their now-unused imports (deferred, text, postgresql JSON); no remaining references to these columns anywhere in src/ or tests/.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant R1 as Release 1 (#3481)
    participant R2 as Release 2 (this PR)
    participant DB as PostgreSQL

    R1->>DB: Mark size/size_by_entrypoint deferred (stop reading)
    R1->>DB: "Move size default to server_default=0 (stop writing)"
    Note over R1,DB: Columns still exist in schema
    Note over R2: Safe to drop: N-1 code no longer references columns
    R2->>DB: DROP COLUMN size_by_entrypoint
    R2->>DB: DROP COLUMN size
    Note over R2,DB: Downgrade recreates both columns in post-R1 state
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant R1 as Release 1 (#3481)
    participant R2 as Release 2 (this PR)
    participant DB as PostgreSQL

    R1->>DB: Mark size/size_by_entrypoint deferred (stop reading)
    R1->>DB: "Move size default to server_default=0 (stop writing)"
    Note over R1,DB: Columns still exist in schema
    Note over R2: Safe to drop: N-1 code no longer references columns
    R2->>DB: DROP COLUMN size_by_entrypoint
    R2->>DB: DROP COLUMN size
    Note over R2,DB: Downgrade recreates both columns in post-R1 state
Loading

Reviews (1): Last reviewed commit: "Drop deprecated lanes.size columns (PP-4..." | Re-trigger Greptile

@codecov

codecov Bot commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.47%. Comparing base (bf23006) to head (014147b).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3563   +/-   ##
=======================================
  Coverage   93.46%   93.47%           
=======================================
  Files         512      512           
  Lines       46611    46609    -2     
  Branches     6352     6352           
=======================================
- Hits        43567    43566    -1     
+ Misses       1968     1967    -1     
  Partials     1076     1076           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DB migration This PR contains a DB migration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant