It is possible to commit a migration without adding the version to schema_migrations

[samsondav]

Environment

• Elixir version (elixir -v):

Erlang/OTP 21 [erts-10.0.7] [source] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:1] [hipe]

Elixir 1.7.3 (compiled with Erlang/OTP 21)

• Database and version (PostgreSQL 9.4, MongoDB 3.2, etc.):

PostgreSQL 9.6

• Ecto version (mix deps):

ecto 3.0.0
ecto_sql 3.0.1

• Database adapter and version (mix deps):

postgres 0.14.0

• Operating system:

Current behavior

Include code samples, errors and stacktraces if appropriate.

Expected behavior

In the process of upgrading to Ecto 3.0, I found that it is possible for Ecto to commit a transaction whilst not updating the schema_migrations table. This is never desirable, since it means that migrations could be run more than once. Migrations should always be atomic.

The error that caused me to notice this was as follows:

Our schema_migrations table is old and was originally migrated from a MySQL instance. For this reason it has column inserted_at with type timestamptz not timestamp as Ecto expects. This was never a problem in Ecto 2.x but in Ecto 3.x it raises an error.

Here is an example migration run:

mix ecto.migrate
[debug] QUERY OK source="schema_migrations" db=6.5ms
SELECT e0."version"::bigint FROM "schema_migrations" AS e0 FOR UPDATE []
[info] == Running 20181107083658 NestDB.Repo.Migrations.UnifyTimestamps.change/0 forward
[info] execute "ALTER TABLE my_table ADD COLUMN this_column_should_never_be_committed"
[info] == Migrated 20181107083658 in 12.2s
[error] Could not update schema migrations. This error usually happens due to the following:

  * The database does not exist
  * The "schema_migrations" table, which Ecto uses for managing
    migrations, was defined by another library

To fix the first issue, run "mix ecto.create".

To address the second, you can run "mix ecto.drop" followed by
"mix ecto.create". Alternatively you may configure Ecto to use
another table for managing migrations:

    config :nest_db, NestDB.Repo,
      migration_source: "some_other_table_for_schema_migrations"

The full error report is shown below.

** (DBConnection.EncodeError) Postgrex expected %DateTime{}, got ~N[2018-11-07 08:57:01]. Please make sure the value you are passing matches the definition in your table or in your query or convert the value accordingly.
    /Users/sam/code/nested/local-stack/elixir-umbrella/deps/postgrex/lib/postgrex/type_module.ex:713: NestDB.PostgrexTypes.encode_params/3
    (postgrex) lib/postgrex/query.ex:62: DBConnection.Query.Postgrex.Query.encode/3
    (db_connection) lib/db_connection.ex:1074: DBConnection.encode/5
    (db_connection) lib/db_connection.ex:1172: DBConnection.run_prepare_execute/5
    (db_connection) lib/db_connection.ex:480: DBConnection.parsed_prepare_execute/5
    (db_connection) lib/db_connection.ex:473: DBConnection.prepare_execute/4
    (postgrex) lib/postgrex.ex:167: Postgrex.query/4
    (ecto_sql) lib/ecto/adapters/sql.ex:627: Ecto.Adapters.SQL.struct/10
make: *** [migrate] Error 1

After this migration run, the SQL has been applied but the version is not present in the schema. Subsequent migrations will run the same SQL again.

Furthermore while trying to fix this I discovered it isn't possible to alter the schema_migrations table inside a migration. It hangs forever.

This leads me to conclude that the transaction and update of schema_migrations happens outside of the main migration transaction. This is not a good design, since it can lead to deadlocks and causes the migration to not commit atomically. Ideally, everything would happen inside the same transaction.

[josevalim]

Good catch!

I have a fix in hand. It is more a bug rather than a design flaw, as we can fix it by literally moving some lines of code into a block. :) I am writing the tests and I should push a fix shortly.

I am wondering if there is something we can do about the timestamptz bit but I am not sure. Previously Ecto would not care about this but the whole point of Elixir Calendar types is to make the distinction between those clear.

[josevalim]

Here is a PR: elixir-ecto/ecto_sql#30

Can you please try it out? I have been a bit too optimistic about the fix but it should be good to go (there were some complexities regarding failure handling with tasks but we have added tests for them too).

[josevalim]

Closing in favor of the PR, we can continue the discussion in the other repo.