Page MenuHomeElementl

Migrate db.String to db.Text to support MySQL

Authored by rexledesma on Jan 4 2021, 10:38 PM.



To support MySQL, we migrate incompatible column types. For example, it does not support a bare db.String, as it is not fixed length.
Here, we migrate all uses of db.String to db.Text to accommodate for this.

Co-authored-by: Rex Ledesma <>

Test Plan

Diff Detail

R1 dagster
Lint Passed
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
nate edited the test plan for this revision. (Show Details)


prha added inline comments.
26–35 ↗(On Diff #29419)

this is an event_log migration, so will probably a no-op.

I'd expect only the event logs migrations here.


these two tables are deprecated so you probably do not need to migrate

26–35 ↗(On Diff #29419)

I'd expect a section here for each storage table that needs to be altered:

if "event_logs" in has_tables:
     # alter event_logs

if "job_ticks" in has_tables:
    # alter job_ticks

8 ↗(On Diff #29419)


rexledesma added a reviewer: nate.
rexledesma removed a reviewer: nate.
rexledesma retitled this revision from RFC - Schema changes for future MySQL support to Migrate db.String to db.Text to support MySQL.Jan 14 2021, 8:16 AM
rexledesma edited the summary of this revision. (Show Details)
rexledesma edited the test plan for this revision. (Show Details)

remove duplicate migration of secondary_indexes

34–38 ↗(On Diff #29703)

Just as a callout, although secondary_indexes is defined in the runs and event_log schema, the table is only altered once in the migration.

314–324 ↗(On Diff #29703)

Just removed some stuff in this test to make it pass

prha requested changes to this revision.Jan 14 2021, 5:22 PM

This looks pretty solid... nice work.

Some minor things to address here, first, though.

32 ↗(On Diff #29703)

I think this should also have a secondary index section... In the sqlite case, it's two separate tables.

306 ↗(On Diff #29703)

Why do we need to upgrade here? I think the point of the test is actually to not upgrade the instance (which will trigger a data migration for run storage).

You might need to change this call to instance._run_storage.upgrade() just to do the schema upgrade. Then I think the rest of the test (without the assert removals) will work.

347 ↗(On Diff #29703)

This is still going to be brittle. I vote to remove this check.

Or just assert that the alembic version != b22f16781a7c

This revision now requires changes to proceed.Jan 14 2021, 5:22 PM
rexledesma edited the summary of this revision. (Show Details)

address comments on test

add back migration script to secondary_indexes for runs schema

okay, looks good to me!


This revision is now accepted and ready to land.Jan 14 2021, 6:06 PM
This revision was automatically updated to reflect the committed changes.