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

create_if_not_exists cannot run concurrently #2703

Closed
novaugust opened this issue Sep 23, 2018 · 8 comments
Closed

create_if_not_exists cannot run concurrently #2703

novaugust opened this issue Sep 23, 2018 · 8 comments

Comments

@novaugust
Copy link
Contributor

novaugust commented Sep 23, 2018

Environment

  • Elixir version (elixir -v): 1.6.4
  • Database and version (PostgreSQL 9.4, MongoDB 3.2, etc.): 10.5
  • Ecto version (mix deps): 2.2.10
  • Database adapter and version (mix deps): postgrex 0.13.5
  • Operating system: mac osx

Current behavior

Using create_if_not_exists to create an index concurrently erroneously raises a compiletime error.

Example migration:

defmodule Hello.Repo.Migrations.CreateIfNotExistsIndexConcurrently do
  use Ecto.Migration

  @disable_ddl_transaction true

  def change do
    create_if_not_exists(index("example", [:field], concurrently: true))
  end
end

mix ecto.migrate raises:

** (Postgrex.Error) ERROR 25001 (active_sql_transaction): CREATE INDEX CONCURRENTLY cannot run inside a transaction block
    (ecto) lib/ecto/adapters/sql.ex:200: Ecto.Adapters.SQL.query!/5
    (ecto) lib/ecto/adapters/postgres.ex:96: anonymous fn/4 in Ecto.Adapters.Postgres.execute_ddl/3
    (elixir) lib/enum.ex:1899: Enum."-reduce/3-lists^foldl/2-0-"/3
    (ecto) lib/ecto/adapters/postgres.ex:96: Ecto.Adapters.Postgres.execute_ddl/3
............

Expected behavior

No error should be raised and the migration should happen, identically to using create

@novaugust novaugust changed the title create_if_exists cannot run concurrently create_if_not_exists cannot run concurrently Sep 23, 2018
@josevalim
Copy link
Member

It is not a compile time error, but a regular error when running migrations. The Ecto.Migration docs cover this too: https://hexdocs.pm/ecto/Ecto.Migration.html#module-transactions

@novaugust
Copy link
Contributor Author

novaugust commented Sep 23, 2018

@josevalim sorry, I mistyped the report. it is indeed a runtime error. brainfart on a late night bug report.

Maybe I didn't do a good enough job of pointing out I did specify @disable_ddl_transaction true in the migration and am still seeing the "cannot run in a transaction" error.
the exact same code will work if you just switch create_if_not_exists to create

@josevalim
Copy link
Member

Maybe I didn't do a good enough job of pointing out I did specify @disable_ddl_transaction true in the migration and am still seeing the "cannot run in a transaction" error.

That was my fault. :) I should have noticed it. I will investigate.

Btw, can you reproduce it in our suite?

@novaugust
Copy link
Contributor Author

Btw, can you reproduce it in our suite?

#2706 =)

@josevalim
Copy link
Member

josevalim commented Sep 26, 2018

@novaugust thanks for the reproduction mechanism.

I have taken a look at this and the reason is that, in order to support earlier PG versions, we are emulating create if not exists with transactions and error handling.

However, once I changed the code to emit a proper "CREATE INDEX IF NOT EXISTS CONCURRENTLY", then the test would never finish. It seems it has to wait until any on going transaction finishes, which conflicts with our new lock that avoids multiple migrations from running at the same time.

The only way I can see for this to work is to have a mechanism to disable the migration lock... but then you are putting yourself at risk of running the migration multiple times at once, while at the same time requiring us to drop support for the operation in more recent PG versions.

For all of those reasons, I am more inclined to raise an error for now. You can bypass the error by doing a execute(...) but then you will run into the migration lock issue.

@josevalim
Copy link
Member

Or maybe you can simulate this by doing a query checking for the index before emitting the command.

@josevalim
Copy link
Member

Closing this as we at at least raise a better error message now thanks to 5f73f43.

@novaugust
Copy link
Contributor Author

not a big deal, I didn't need the migration to work that way, so I just sidestepped it and thought i'd report it =)

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

No branches or pull requests

2 participants