move migration table updating SQL into a migration transaction

[xtremerui]

What does this PR accomplish?

Bug Fix | Feature | Documentation

Fix #6695

Changes proposed by this PR:

we should not rely on Go to execute the SQL to update migrations_history table as the process could be killed before it could run the query, which will make Concourse lost track of migration state (it relies on the entry in migrations_history table).

Instead, we could put the SQL into each migration itself. In this way if the migration is done, the migrations_history table is guaranteed to be updated.

Notes to reviewer:

we have tried to intercept the os kill signal so Go could run a clean up but it seems Go could only catch syscall.SIGTERM (gracefully shutdown with draining). It can't catch syscall.SIGKILL, unfortunatelly.

Release Note

Fix a bug where a completed migration was not recorded in migrations_history table

Contributor Checklist

• Followed [Code of conduct], [Contributing Guide] & avoided [Anti-patterns]
• [Signed] all commits
• Added tests (Unit and/or Integration)
• Added release note (Optional)

Reviewer Checklist

• Code reviewed
• Tests reviewed
• Release notes reviewed
• PR acceptance performed

[aoldershaw]

It seems like it could be pretty easy to make mistakes if we need to put the INSERT INTO migrations_history line in each transaction, especially when reordering migrations. We could probably add a test for this to ensure things stay correct, but I wonder if there's a better way.

What if we just enforced all migrations to run in a transaction? In runMigration, we start a new transaction, and get rid of the BEGIN/COMMIT in each of the SQL transactions. In the go transactions, we could pass in the db.Tx rather than the db.Conn, and remove the start/rollback/commit logic from each transaction.

This is more restrictive in that we can no longer choose to run a migration outside of a transaction, but I'm not too sure what use-cases there are for doing so anyway - looks like all our migrations (besides empty ones) use a transaction.

Does that make sense?

[xtremerui]

@aoldershaw we have updated the PR so that migrator use a tx instead of conn. All .go and .sql migrations has to be updated accordingly. We removed BEGIN;COMMIT; from sql migration and removed Commit() Rollback() in go migration. For you convinient you could view the changes by commit to skip the migrations file changes.

[vito]

This is more restrictive in that we can no longer choose to run a migration outside of a transaction, but I'm not too sure what use-cases there are for doing so anyway - looks like all our migrations (besides empty ones) use a transaction.

This came up in retro and I brought up needing to be able to do this, but I don't think we need to anymore. The migration that needed it before has since been squashed into the new initial schema.

There was a time when we had to do an ALTER TYPE ... ADD VALUE, which can't be run in a transaction in PostgreSQL 9.5.

ALTER TYPE ... ADD VALUE (the form that adds a new value to an enum type) cannot be executed inside a transaction block.

But as of Postgres 12, it can!

If ALTER TYPE ... ADD VALUE (the form that adds a new value to an enum type) is executed inside a transaction block, the new value cannot be used until after the transaction has been committed.

I actually looked into this recently and I'm fairly certain this was the only type of query that could not be run in a transaction. Given that we don't do this often (last time we did it was to add another volume/container state iirc), if/when it does come up again I'd rather take that as an opportunity to raise the minimum version to 12 so we can leverage more shiny new features.

So I'm good with just forcing transactions everywhere. 👍

[aoldershaw]

LGTM