feat: run all migrations in a transaction #10966

coadler · 2023-11-30T22:23:23Z

Updates https://github.com/coder/customers/issues/365

This PR updates our migration framework to run all migrations in a single transaction. This is the same behavior we had in v1 and ensures that failed migrations don't bring the whole deployment down. If a migration fails now, it will automatically be rolled back to the previous version, allowing the deployment to continue functioning.

coadler · 2023-11-30T22:23:35Z

Current dependencies on/for this PR:

main
- PR feat: run all migrations in a transaction #10966 👈

This stack of pull requests is managed by Graphite.

johnstcn · 2023-12-01T08:55:36Z

coderd/database/migrations/000057_api_key_token.up.sql

@@ -1 +1,22 @@
-ALTER TYPE login_type ADD VALUE IF NOT EXISTS 'token';
+-- ALTER TYPE login_type ADD VALUE IF NOT EXISTS 'token';


add comment explaining why we need to do the below now instead

johnstcn · 2023-12-01T08:56:17Z

coderd/database/migrations/000063_resource_type_group.up.sql

-ALTER TYPE resource_type ADD VALUE IF NOT EXISTS 'group'; 
-
-COMMIT;
+ALTER TYPE resource_type ADD VALUE IF NOT EXISTS 'group';


Should we also do the new table + rename dance here as well?
Seems safer to just always do it that way instead of someone suddenly adding a new migration referencing this enum type and wondering why their new migration doesn't work.

I only went back historically and fixed usage of ADD VALUE if we reference the newly added value later in a different migration. It is otherwise fine to use within a transaction.

Edit: Ah sorry I didn't fully read. I see what you mean but I wasn't sure if it was worth going back to every type. It was really a pain to try and figure out for each enum what the exact values were at the time of the migration. I don't really mind following up though.

I'll fix all of them in a follow up 👍

johnstcn · 2023-12-01T08:58:28Z

coderd/database/migrations/000134_workspace_build_reason.up.sql

 ALTER TYPE build_reason ADD VALUE IF NOT EXISTS 'autolock';
 ALTER TYPE build_reason ADD VALUE IF NOT EXISTS 'failedstop';
 ALTER TYPE build_reason ADD VALUE IF NOT EXISTS 'autodelete';


new table + rename dance here too

johnstcn · 2023-12-01T08:58:47Z

coderd/database/migrations/000149_agent_multiple_subsystems.up.sql

@@ -1,5 +1,3 @@
-BEGIN;
-
 -- Add "exectrace" to workspace_agent_subsystem type.
 ALTER TYPE workspace_agent_subsystem ADD VALUE 'exectrace';


new table + rename

johnstcn · 2023-12-01T09:00:01Z

coderd/database/migrations/000153_agent_default_apps.down.sql

-DROP TYPE display_app; 
-COMMIT;
-
+DROP TYPE display_app;


Does dropping a type also potentially have the same issue as adding a type?

Since it's dropping the entire type it doesn't have the same consequences as ADD VALUE.

mafredri · 2023-12-01T12:08:28Z

It would be pretty nice if we could code-gen the in-transaction type modifications. Code-gen would essentially apply migrations without transactions, and when there's a type-change, it generates code for the new type/rename dance on all tables using the type. It might be somewhat nice to implement our own custom SQL here as well for managing types (ALTER TYPE .. DROP VALUE), a bit strange, granted 😂.

Are types/enums the only issue we may run into in migrations running under a single tx?

deansheather · 2023-12-01T14:13:09Z

coderd/database/migrations/create_migration.sh

+safer, but means that 'ALTER TYPE resource_type ADD VALUE' cannot be used if the
+enum value needs to be referenced in another migration.


You should document how to work around this limitation

Documentation alone is not sufficient IMO (hence suggesting code-gen). This is the type of issue that might surface a week, month, or year later when a migration using the earlier type is added. And only some customers will hit it (those that rarely update).

This isn't really a bug that customers will run into, it'll always be caught by tests. The issue only pops up when you attempt to use an enum value added by ADD VALUE in a later migration. Otherwise, these are fine to use.

I don't think we can always catch this as it is now, (e.g. without codegen, linting, etc). One scenario I can think of is:

Customer A and B run v2.0.0

We add a value (foo(bar)) in v2.0.1, but we forget the type dance because there's nothing to catch it when the value is unused

Customer B updates to v2.0.1

We release v2.0.2 and add a migration that modifies rows and change foo(baz) => foo(bar), we don't catch it because we don't have fixtures for everything and the relevant tables are empty

Customer A and B update to v2.0.2

Customer A runs into an error

The migration tests will catch this since it doesn't matter if the tables are empty. Any reference to the new enum will fail. I ran into this exact scenario while writing this.

https://github.com/coder/coder/blob/main/coderd/database/migrations/000103_add_apikey_name.up.sql does an UPDATE using the login_type(token) enum added using ADD VALUE in an earlier migration. The tests are able to catch this without fixtures since it doesn't matter if any rows are in the table, it still has to convert 'token' to the proper enum which causes the error.

Oh ok, that's good then. I wasn't aware that postgres would catch it for an empty table. Thanks for clarifying. 👍🏻

coadler · 2023-12-01T17:28:41Z

@mafredri

It would be pretty nice if we could code-gen the in-transaction type modifications. Code-gen would essentially apply migrations without transactions, and when there's a type-change, it generates code for the new type/rename dance on all tables using the type. It might be somewhat nice to implement our own custom SQL here as well for managing types (ALTER TYPE .. DROP VALUE), a bit strange, granted 😂.

I think it would be cool, but I'm not sure entirely worthwhile. The whole DROP TYPE & CREATE TYPE dance is pretty copy-pastable and doesn't take more than a few minutes. Given how infrequently these happen I think this is a fine trade-off.

Are types/enums the only issue we may run into in migrations running under a single tx?

In terms of Postgres limitations, yes. They only cause an issue if you ADD VALUE and then try to reference that same value in a later migration. There are other limitations though such as making two temp tables with the same name in different migrations, but those are more semantics.

github-actions bot assigned coadler Nov 30, 2023

coadler force-pushed the colin/featrunallmigrationsinatransaction branch from 5a5be7b to 8c20d9d Compare November 30, 2023 22:33

coadler marked this pull request as ready for review December 1, 2023 05:48

coadler requested review from johnstcn and deansheather December 1, 2023 05:52

johnstcn reviewed Dec 1, 2023

View reviewed changes

deansheather approved these changes Dec 1, 2023

View reviewed changes

coadler added 9 commits December 1, 2023 21:26

feat: run all migrations in a transaction

b49ee44

fix migrations

31524f3

lint

785ff59

fmt

9a87f47

revert

d9b7db6

add comments

307b999

hold migration lock on migration table creation

0cb53fd

remove comment

480b99d

document the correct way to create a new enum value

3ca5ebf

coadler force-pushed the colin/featrunallmigrationsinatransaction branch from 423df44 to 3ca5ebf Compare December 1, 2023 21:32

coadler merged commit 8e684c8 into main Dec 1, 2023
22 checks passed

coadler deleted the colin/featrunallmigrationsinatransaction branch December 1, 2023 22:11

github-actions bot locked and limited conversation to collaborators Dec 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: run all migrations in a transaction #10966

feat: run all migrations in a transaction #10966

coadler commented Nov 30, 2023 •

edited

coadler commented Nov 30, 2023

johnstcn Dec 1, 2023

johnstcn Dec 1, 2023

coadler Dec 1, 2023 •

edited

coadler Dec 1, 2023

johnstcn Dec 1, 2023

johnstcn Dec 1, 2023

johnstcn Dec 1, 2023

coadler Dec 1, 2023

mafredri commented Dec 1, 2023 •

edited

deansheather Dec 1, 2023

mafredri Dec 1, 2023

coadler Dec 1, 2023

mafredri Dec 1, 2023 •

edited

coadler Dec 1, 2023 •

edited

mafredri Dec 1, 2023

coadler commented Dec 1, 2023

		@@ -1 +1,22 @@
		ALTER TYPE login_type ADD VALUE IF NOT EXISTS 'token';
		-- ALTER TYPE login_type ADD VALUE IF NOT EXISTS 'token';

		safer, but means that 'ALTER TYPE resource_type ADD VALUE' cannot be used if the
		enum value needs to be referenced in another migration.

feat: run all migrations in a transaction #10966

feat: run all migrations in a transaction #10966

Conversation

coadler commented Nov 30, 2023 • edited

coadler commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coadler Dec 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mafredri commented Dec 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mafredri Dec 1, 2023 • edited

Choose a reason for hiding this comment

coadler Dec 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coadler commented Dec 1, 2023

coadler commented Nov 30, 2023 •

edited

coadler Dec 1, 2023 •

edited

mafredri commented Dec 1, 2023 •

edited

mafredri Dec 1, 2023 •

edited

coadler Dec 1, 2023 •

edited