Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrations are not idempotent and cause critical divergence between applies #92

Closed
aeneasr opened this issue Jun 10, 2020 · 2 comments · Fixed by #94
Closed

Migrations are not idempotent and cause critical divergence between applies #92

aeneasr opened this issue Jun 10, 2020 · 2 comments · Fixed by #94

Comments

@aeneasr
Copy link
Member

aeneasr commented Jun 10, 2020

This is a follow up to #91. Upon further debugging, I have made the observation that instead of translating fizz migrations 1:1 to SQL statements, pop/fizz tries to be smart and bundles together the SQL statements, creating one SQL "file" that has "optimizations" (such as removing alter tables in favor of just creating the table). This of course is very bad because if bugs (such as #91) happen, they actually depend on the existing state of the database. For example, a fresh database is missing the foreign key while a database with migrations partially applied does not.

Another serious issue is that fixing bugs like #91 in fizz will cause divergence of databases because new databases (without any existing state) will properly create the foreign key, while older databases (with the buggy statement) will not. Bringing them into line is basically a huge trial and error/guessing game based on what SQL statements are used when and where and how and what database has already had which migrations applied.

I can not state enough that this is really bad for any system that aims to have reliable SQL migrations - so far that we are definitely going to have to switch to raw SQL and tell our users to level their existing database because there's too much complexity involved in fixing all these nuances.

But there is a path forward in my opinion: Fizz (or pop) should use the fizz templates to generate SQL files for each database, which then should be checked into version control and be applied by the migration logic instead of using fizz on-the-fly. This means that fizz would basically become a CLI command that could be used with e.g. go generate along the lines of:

fizz migrations/*.fizz sql_migration_output/

That would then be a directory along the lines of:

  • sql_migration_output
    • 123123_myfizzmigration.mysql.sql
    • 123123_myfizzmigration.postgres.sql
    • 123123_myfizzmigration.sqlite.sql

and so on.

As an intermediate step I would highly encourage to put a big warning in the docs to not use fizz migrations because of these issues. Whatever you do, you will run into this stuff eventually and when you do - and depending on when - you're pretty much fucked, especially if it's another OSS project used by people against many, many databases.

@aeneasr
Copy link
Member Author

aeneasr commented Jun 17, 2020

I have to take back one of my criticisms, after further debugging and inspection it does not actually appear that there are "optimizations" being done. Instead, fizz files are translated 1:1 to SQL. This happens here. I was confused by the SQL dump fizz/pop is doing when migration fails - but it appears that the export is happening using the database's "native" SQL dump tool (e.g. pg_dump).

This however doesn't of course resolve backwards incompatible changes to the fizz language, which will lead to divergence of generated SQL over time and can cause migrations to fail when - for example - a bug such as #91 is fixed.

@aeneasr
Copy link
Member Author

aeneasr commented Jun 18, 2020

To address this issue in our ecosystem I have implemented a small CLI helper that renders fizz templates to raw SQL, which we check into VCS and disallow changes to. You can find the CLI helper and related changes here: https://github.com/ory/cli/pull/2/files

aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 19, 2020
This patch adds e2e migration tests with multiple changes to the database schema and their expected output. Currently only supports SQLite migrations but the plan is to add more tests over time.

This suite includes a failing test case for gobuffalo#92
aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 19, 2020
This patch adds e2e migration tests with multiple changes to the database schema and their expected output. Currently only supports SQLite migrations but the plan is to add more tests over time.

This suite includes a failing test case for gobuffalo#92
aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 20, 2020
This patch adds e2e migration tests with multiple changes to the database schema and their expected output. Currently only supports SQLite migrations but the plan is to add more tests over time.

This suite includes a failing test case for gobuffalo#92

unstaged

unstaged
aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 20, 2020
This patch adds e2e migration tests with multiple changes to the database schema and their expected output. Currently only supports SQLite migrations but the plan is to add more tests over time.

This suite includes a failing test case for gobuffalo#92

unstaged

unstaged
aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 20, 2020
This patch resolves issues with `change_column` and `drop_column` which previously caused duplicate or missing indices and foreign keys. To prevent future regressions and help debugging,
a new e2e-test pipeline has been added to the project and the CI. Documentation for running the tests has been added to the README.

End-to-end tests currently support MySQL, PostgreSQL, and SQLite and do not yet cover other SQL flavors.

During review, please help ensure that the SQL fixtures are not missing something :)

Closes gobuffalo#92
aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 20, 2020
This patch resolves issues with `change_column` and `drop_column` which previously caused duplicate or missing indices and foreign keys. To prevent future regressions and help debugging,
a new e2e-test pipeline has been added to the project and the CI. Documentation for running the tests has been added to the README.

End-to-end tests currently support MySQL, PostgreSQL, and SQLite and do not yet cover other SQL dialects.

During review, please help ensure that the SQL fixtures are not missing something :)

Closes gobuffalo#92
aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 20, 2020
This patch resolves issues with `change_column` and `drop_column` which previously caused duplicate or missing indices and foreign keys. To prevent future regressions and help debugging,
a new e2e-test pipeline has been added to the project and the CI. Documentation for running the tests has been added to the README.

End-to-end tests currently support MySQL, PostgreSQL, and SQLite and do not yet cover other SQL dialects.

During review, please help ensure that the SQL fixtures are not missing something :)

Closes gobuffalo#92
aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 20, 2020
This patch resolves issues with `change_column` and `drop_column` which previously caused duplicate or missing indices and foreign keys. To prevent future regressions and help debugging,
a new e2e-test pipeline has been added to the project and the CI. Documentation for running the tests has been added to the README.

End-to-end tests currently support MySQL, PostgreSQL, and SQLite and do not yet cover other SQL dialects.

During review, please help ensure that the SQL fixtures are not missing something :)

Closes gobuffalo#92
aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 20, 2020
This patch resolves issues with `change_column` and `drop_column` which previously caused duplicate or missing indices and foreign keys. To prevent future regressions and help debugging,
a new e2e-test pipeline has been added to the project and the CI. Documentation for running the tests has been added to the README.

End-to-end tests currently support MySQL, PostgreSQL, and SQLite and do not yet cover other SQL dialects.

During review, please help ensure that the SQL fixtures are not missing something :)

Closes gobuffalo#92
aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 20, 2020
This patch resolves issues with `change_column` and `drop_column` which previously caused duplicate or missing indices and foreign keys. To prevent future regressions and help debugging,
a new e2e-test pipeline has been added to the project and the CI. Documentation for running the tests has been added to the README.

End-to-end tests currently support MySQL, PostgreSQL, and SQLite and do not yet cover other SQL dialects.

During review, please help ensure that the SQL fixtures are not missing something :)

Closes gobuffalo#92
aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 20, 2020
This patch resolves issues with `change_column` and `drop_column` which previously caused duplicate or missing indices and foreign keys. To prevent future regressions and help debugging,
a new e2e-test pipeline has been added to the project and the CI. Documentation for running the tests has been added to the README.

End-to-end tests currently support MySQL, PostgreSQL, and SQLite and do not yet cover other SQL dialects.

During review, please help ensure that the SQL fixtures are not missing something :)

Closes gobuffalo#92
aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 21, 2020
This patch resolves issues with `change_column` and `drop_column` which previously caused duplicate or missing indices and foreign keys. To prevent future regressions and help debugging,
a new e2e-test pipeline has been added to the project and the CI. Documentation for running the tests has been added to the README.

End-to-end tests currently support MySQL, PostgreSQL, and SQLite and do not yet cover other SQL dialects.

During review, please help ensure that the SQL fixtures are not missing something :)

Closes gobuffalo#92
aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 21, 2020
This patch resolves issues with `change_column` and `drop_column` which previously caused duplicate or missing indices and foreign keys. To prevent future regressions and help debugging,
a new e2e-test pipeline has been added to the project and the CI. Documentation for running the tests has been added to the README.

End-to-end tests currently support MySQL, PostgreSQL, and SQLite and do not yet cover other SQL dialects.

During review, please help ensure that the SQL fixtures are not missing something :)

Closes gobuffalo#92
aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 21, 2020
This patch resolves issues with `change_column` and `drop_column` which previously caused duplicate or missing indices and foreign keys. To prevent future regressions and help debugging,
a new e2e-test pipeline has been added to the project and the CI. Documentation for running the tests has been added to the README.

End-to-end tests currently support MySQL, PostgreSQL, and SQLite and do not yet cover other SQL dialects.

During review, please help ensure that the SQL fixtures are not missing something :)

Closes gobuffalo#92
aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 21, 2020
This patch resolves issues with `change_column` and `drop_column` which previously caused duplicate or missing indices and foreign keys. To prevent future regressions and help debugging,
a new e2e-test pipeline has been added to the project and the CI. Documentation for running the tests has been added to the README.

End-to-end tests currently support MySQL, PostgreSQL, and SQLite and do not yet cover other SQL dialects.

During review, please help ensure that the SQL fixtures are not missing something :)

Closes gobuffalo#92
aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 21, 2020
This patch resolves issues with `change_column` and `drop_column` which previously caused duplicate or missing indices and foreign keys. To prevent future regressions and help debugging,
a new e2e-test pipeline has been added to the project and the CI. Documentation for running the tests has been added to the README.

End-to-end tests currently support MySQL, PostgreSQL, and SQLite and do not yet cover other SQL dialects.

During review, please help ensure that the SQL fixtures are not missing something :)

Closes gobuffalo#92
aeneasr added a commit to aeneasr/fizz that referenced this issue Jun 21, 2020
This patch resolves issues with `change_column` and `drop_column` which previously caused duplicate or missing indices and foreign keys. To prevent future regressions and help debugging,
a new e2e-test pipeline has been added to the project and the CI. Documentation for running the tests has been added to the README.

End-to-end tests currently support MySQL, PostgreSQL, and SQLite and do not yet cover other SQL dialects.

During review, please help ensure that the SQL fixtures are not missing something :)

Closes gobuffalo#92
stanislas-m pushed a commit that referenced this issue Jun 23, 2020
…94)

This patch resolves issues with `change_column` and `drop_column` which previously caused duplicate or missing indices and foreign keys. To prevent future regressions and help debugging,
a new e2e-test pipeline has been added to the project and the CI. Documentation for running the tests has been added to the README.

End-to-end tests currently support MySQL, PostgreSQL, and SQLite and do not yet cover other SQL dialects.

During review, please help ensure that the SQL fixtures are not missing something :)

Closes #92
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant