Separating the partial-rollback from roll forward in migration scripts

[c42f]

Problem statement / motivation

As a new user introducing graphile-migrate to a team, I've noticed that code review of migrations is complicated by the need to thoroughly check the DROP statements that are required for idempotency. This is a point of friction for introducing the tooling, as people responsible for operational databases are nervous about having migrations containing a lot of DROP statements. It seems we currently have the following tradeoff

• If someone writes an errant DROP statement for a resource in the existing schema, data loss will occur in production.
• However, various DROP statements are a necessary part of idempotency and having graphile-migrate watch work during development.

Feature description

I find that it's often possible to separate migrations into two halves which seem like they could be treated differently in production and development:

1. The DROP (or other) statements which remove any resources previously created during development. This is like a partial rollback which is happy to destroy data, and only needs to get the DB in a state where the roll forward phase can reliably be reapplied.
2. The roll forward migration statements needed for production.

If graphile-migrate guaranteed to only apply (2) in production (and for the shadow db rollforward check phase) it would seem unnecessary for (1) to be thoroughly vetted in code review, and reduce overall risk of applying a migration.

I'm not sure what this feature would look like. Could it be a comment which separates migration files into two sections? That seems a little fragile (eg, what if the comment is spelled wrong?), but it would be minimally breaking.

Supporting development

I could potentially help build this.

• [ ] am interested in building this feature myself
• [x] am interested in collaborating on building this feature
• [x] am willing to help testing this feature before it's released
• [ ] am willing to write a test-driven test suite for this feature (before it exists)
• [ ] am a Graphile sponsor ❤️
• [ ] have an active support or consultancy contract with Graphile

[benjie]

This is already possible if you’re disciplined; use current/001-rollback.sql for your rollbacks of the new features and current/100-migration.sql for everything else (including drops of functions/constraints/etc that you are replacing); then when you are ready:

1. stop running watch
2. run the rollback against your local DB (graphile-migrate run migrations/current/001-rollback.sql or similar)
3. Delete or comment out 001-rollback.sql
4. Commit the migration

GM’s migrations only run once on commit/migrate so the idempotency is only required for watch mode.

You might be able to use a beforeCommit action to perform this procedure; if that action doesn’t already exist then a PR to add it would be welcome (with the associated afterCommit).

[c42f]

Thanks for the pointers. There's a key part which I don't understand in this suggestion: how do we have 001-rollback.sql tracked by the system, but not applied to the production database?

I guess we could have our custom beforeCommit delete the 001-rollback.sql after applying it to the development database so that it never makes it into the committed version. That would be ok I guess (provided 001-rollback.sql is tracked in git for recovery if the commit fails for some reason), but it feels like a bit of a hack and likely to lead to surprises.

[benjie]

Oops, yes - I meant delete it.

Another potential option is to have a rollback script we call via beforeCurrent and maybe blank that out or move it somewhere afterCommit?

Basically I’m not sure currently how to best achieve this in the migrations themselves, I don’t like the idea of inline comments because our migration/current files should be runnable with psql without needing custom parsing (note the : placeholders we use are inspired by psql). However our lifecycle hooks should allow you to design a flow you’re happy with, and if they do not then we should enhance them.

[c42f]

I don’t like the idea of inline comments because our migration/current files should be runnable with psql without needing custom parsing

Yes that makes sense.

The thing I don't love about deleting the rollback is that it becomes quite ephemeral; I'd prefer to have prior rollbacks as reference material when developing future migrations.

Though with that in mind, maybe it makes sense to have the beforeCommit hook simply comment out the portions which are deemed to be rollbacks. There's any number of ways that could be achieved with a custom script.

[benjie]

Of course, your beforeCommit could also figure out what the next commit number will be (we could even expose it in an envvar) and move rather than delete e.g. migrations/rollbacks/000036.sql.

We could even have an afterUncommit that does the reverse. (I don't think we have uncommit hooks currently, but they wouldn't be much work to add I suspect.)

[c42f]

we could even expose it in an envvar and move rather than delete

True! I think this is the right idea.