Add helpful error when pool size is too small

[blatyo]

No description provided.

[blatyo]

What would be the best way to test this? The transaction checkout in the migration task would raise the exception that this is providing a more descriptive error for.

[fishcakez]

Unfortunately this would only work with Ecto.Adapters.SQL.Sandbox and not the other pools. Also we don't want to be pattern matching on the string message of the DBConnection.ConnectionError struct because there is no guarantee that we match on correct messages or that message will stay the same.

[josevalim]

It is probably better to check the pool size on Ecto.Migrator or on the "ecto.migrate" task.

[fishcakez]

Are we not able to make it work with single connection by default?

[josevalim]

@fishcakez copying from the other comment in case some folks find this question first:

It is because we need one transaction to hold the lock and another transaction to run the migrations. The reason we cannot do everything in one transaction is because some commands like ALTER TYPE cannot be run on a transaction, so the migration needs the ability to control what happens in a transaction and what does not.

[josevalim]

@blatyo I think instead of trying to wrap the error message, we should proactively check the pool_size in those two places:

• ecto/lib/mix/ecto.ex

  Line 101 in 42d47a6

  pool_size = Keyword.get(opts, :pool_size, 2)

  - and raise if pool_size is 1. In this case, the error message should instruct to set --pool-size to another number

• ecto/lib/mix/ecto.ex

  Line 105 in 42d47a6

  {:error, {:already_started, _pid}} ->

  - this is only invoked if the repository was already started so our pool size config has no effect. In this case, we should check for something like Keyword.fetch(repo.config, :pool_size) == {:ok, 1} and raise if so. In this case, the error message should instruct to change the pool_size in the config/config.exs file

What do you think?

[blatyo]

@josevalim I was trying to only show an error when there was actually an issue. The SQLite adapter could run with pool_size 1 and be fine.

Although, since it's being defaulted to 2 and very few people would configure their repo with less than two, I guess it's not too bad to error on 1. I'll put something up later tonight.

[blatyo]

@fishcakez Like José said, but put a different way, it could be made to work with a single connection as long as you never disabled transactions on a migration.

[josevalim]

@blatyo so maybe we could check for the pool size on lock_for_migration? If set, the pool size is in default_opts (if not set, it defaults to a number > 1, so we don't need to worry): https://github.com/elixir-ecto/ecto/blob/master/lib/ecto/adapters/sql.ex#L606

[blatyo]

@josevalim Is it possible at that point to just ask the pool how many connections it has rather than looking at the options?

[josevalim]

@blatyo I don't think we can ask the pool, there is no such API in DBConnection, but they should all respect the pool_size option.

[fishcakez]

@josevalim we have a pool that is always size 1 (DBConnection.Connection, its actually pool I use the most with Ecto at the moment). We can investigate a pool status call to pick this up.

[josevalim]

@fishcakez that's a great.

Since the goal of the error is to provide a better error message, we don't need to be 100% accurate though. So I would move on with an opts check for now and replace with a status check later on.

What do you think?

[fishcakez]

What do you think?

This sounds good. I expect very few people use this pool because they would need to dive into DBConnection and also not need more than 1 connection so I am not worried about it for a major version change in Ecto. It might be easier if we warn or crash on the single connection pool if pool_size is set other than 1 in DBConnection.Connection itself, and of course do the warning here too.

[blatyo]

I've pushed a new commit that should address everything discussed. I'm unsure where to add a test, as it seems not much in the adapter sql is directly tested.

[josevalim]

We can probably test it on integration_test/sql/migration.exs. Note we will need to start a pool exclusively for this test and then trigger the migration command.

[sourcelevel-bot]

Most of the time you are using the multiple single line alias/require/import/use directives but here you are using the multi-alias/require/import/use syntax

[blatyo]

I've added a test case. AFAIK, the failing test is not related to the changes I've made.

[josevalim]

❤️ 💚 💙 💛 💜