Ecto incompatibilities

[fishcakez]

This issue is to record changes required for Ecto integration.

• [x] Ecto option pool is called pool_mod in Ecto. Solution: rename in DBConnection to pool and proxy_mod to proxy.
• [x] Ecto option pool_timeout is called queue_timeout. Solution: rename in DBConnection as is more consistent and should be used in poolboy pool for sync stopping
• [x] Ecto option max_overflow is called pool_overflow. Consider (soft?) deprecate in Ecto as the naming is inconsistent with other options.
• [x] DBConnection depends on poolboy "~> 1.5" and Ecto depends on poolboy "~> 1.4". Bump the requirement/lock in Ecto as poolboy 1.5.0 has critical fix.

[josevalim]

I agree with all fixes.

[josevalim]

Pushed fixes for the last two to Ecto.

[fishcakez]

• [x] Ecto taints connections on nested transaction rollback/raise and DBConnection does not.

Solution:

• Rollback on rollback call, mark transaction as failed and then throw to transaction try.
• Catch in nested transactions and if not failed rollback on catch, mark transaction as failed and reraise.
• If failed, raise on attempts to use connection until outer transaction ends.
• If failed, after transaction return {:error, :rollback}[1]

[1] Why is it :rollback and not the reason for the rollback?

[josevalim]

We return :rollback only in the case we rollback without an explicit Repo.rollback call. However, we can change it to anything you want as this is meant for Ecto 2.0.

[fishcakez]

I don't mind either way so lets keep the same behaviour. I just wanted to know why :P.

[fishcakez]

• [x] Ecto uses pool_name and DBConnection uses name to name the pool process on start. Solution: Leave as is and rewrite in Ecto.
• [x] Ecto has :shutdown to set the shutdown strategy of connection processes and DBConnection does not. Solution: Add :shutdown to Sojourn pool as Poolboy does not allow setting this.

[fishcakez]

• [x] Ecto logs after checking in process and DBConnection does not provide a proxy callback after check in. Solution: Add terminate/2 proxy callback with reason :normal, {:disconnect, err} or {:stop, reason} depending on check in method.

[josevalim]

I don't mind either way so lets keep the same behaviour. I just wanted to know why :P.

We could include a stacktrace but because we are rolling back only because someone internally called Repo.rollback inside an inner transaction, I am not sure that stacktrace makes sense. :)

[fishcakez]

I've done the DBConnection changes except the transaction rollback change. Down to 7 failing tests in Ecto, 2 of which are due to incorrect rollback behaviour.

[fishcakez]

Transaction rollback behaviour now matches Ecto.

[fishcakez]

The Ecto branch needs a bit more work but is down to 1 failing test: https://github.com/fishcakez/ecto/tree/db_connection