Ecto Persistence: Customize UPSERT options for MySQL adapter

[stewart]

ecto_sql supports UPSERT operations for both the MySQL and Postgres adapters. Postgres requires the conflict_target option be supplied when performing updates, while MySQL does not support it at all.

To allow supporting both of these backends, we'll use the Repo's adapter to differentiate what kind of database we're talking to, and use that to generate the appropriate insert options.

This addresses #40. As mentioned in that issue, .travis.yml changes are required to begin testing against both MySQL and Postgres backends - I wasn't fully comfortable making those changes myself, as the existing build matrix and test setup is reasonably complex.

[tompave]

Hi, thanks for the PR :-)

All the non-ecto build tasks are failing with this:

warning: function FunWithFlags.NullEctoRepo.__adapter__/0 is undefined or private
  lib/fun_with_flags/store/persistent/ecto.ex:191
The command "mix compile --warnings-as-errors" exited with 1.

You'll need to add a stubbed function to NullEctoRepo.

It's only a problem in CI because the persistence modules are condiditionally compiled, only if Ecto or Redix are present. So they can be skipped in an app, but both packages are present in CI.

[stewart]

Apologies for the delayed feedback loop, Travis has been having some issues today. The updated build's just finished, with a couple failures that don't appear to be related to this patch - both of the jobs are using the Redis persistence adapter.

It's possible these are one-off failures related to Travis' partial outage, but I can't ask Travis to retry the build without making an additional commit - could you hit the Retry button when you have a moment?

[tompave]

Yup, they looked like flakeys. I've restarted them and now everything is green.

This seems to work 👍
Have you tried this branch in your project using MySQL?

I'll tweak the Travis matrix after this is merged.

[stewart]

Yep, it's behaving as expected with this change.

[tompave]

Nice. Thank you for working on this. 👍
I need to find some time to test it locally first, but it looks like this is good to be merged.

Before releasing a new version, I want to find a way to get the Ecto tests to run with both Postgres and MySQL in CI.

[tompave]

Testing it locally with MySQL 8.0.15 and a Phoenix app using fun_with_flags_ui.

When I try to add a percentage gate I get this error, reported through Phoenix:

(1064): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'IN SHARE ROW EXCLUSIVE MODE' at line 1

[tompave]

What version of MySQL are you using?

[stewart]

I'm not actually certain, or able to easily tell - this app's hosting situation is somewhat opaque to the development team, for multiple reasons (part of the reason we need feature flags!).

Seems like that's an issue with the in-transaction table lock for the percentage gate put operation. I'm not immediately certain what a direct translation of that to MySQL looks like (I'm more used to Postgres, personally), but am actually concerned it could lead to a deadlock -at least in MySQL, explicit locks are not transaction-safe.

We're not planning to use the percentage flag functionality anytime soon, so perhaps that can be filed as a separate issue and a fix devised there?

[tompave]

I'm not actually certain, or able to easily tell - this app's hosting situation is somewhat opaque to the development team, for multiple reasons.

I see. You can try with this:

SHOW VARIABLES LIKE "%version%";

If you don't have access to the mysql client, you can run it with iex -S mix in your Phoenix app:

Ecto.Adapters.SQL.query!(YourApp.Repo, "SHOW VARIABLES LIKE "%version%";")

[tompave]

Seems like that's an issue with the in-transaction table lock for the percentage gate put operation.

Yes, it's definitely an issue with this:
https://github.com/tompave/fun_with_flags/blob/v1.2.1/lib/fun_with_flags/store/persistent/ecto.ex#L183-L188

The table lock is required because the unique three-column index alone wouldn't prevent duplicated rows for the percentage gates. A different table design would make the table lock not necessary, but when I added the percentage gates I was mainly concerned with keeping it a small and retro-compatible version update. Perhaps it's something I could revisit for version 2.0.

I'm not immediately certain what a direct translation of that to MySQL looks like (I'm more used to Postgres, personally), but am actually concerned it could lead to a deadlock -at least in MySQL, explicit locks are not transaction-safe.

I'm more a Postgres person too. I've looked at the MySQL docs for table locking, and it's not really clear to me yet what the right pattern is. The fact that locks are not released when a transaction commits or rolls back is concerning.

We're not planning to use the percentage flag functionality anytime soon, so perhaps that can be filed as a separate issue and a fix devised there?

I see your point, but in that case I suggest you use your fork instead of getting this partial fix merged. Since with this alone I can't really claim that "it now works with MySQL", I wouldn't release a new version for it -- I wouldn't even keep it on the master branch, but on a mysql_fixes feature branch. So even if I merged this PR, you'd still have to use a git source for the package declaration in your Mixfile, and not pull it from Hex.

The reason is that this is the first time I hear about this problem. I suspect that the reason is most people are using this package with either Redis or Ecto+Postgres; if anyone else tried to use it with MySQL in the past and got blocked because of the problem you encountered, they didn't bother reporting it here on GitHub.

At the moment the situation seems to be that most users of the package can use it fine with their set up, and I could just add to the docs that the Ecto adapter works with Postgres only, even though it's not ideal. On the other hand, if I were to merge this as it is, I wouldn't really consider it ready for release. The changelog entry would be: "now it partially works with MySQL, but not everything". 🤔

So I'd rather either fix everything or nothing at all.

I'll now create a new mysql_fixes branch that you could use as target of this PR, where I'll also add support to run the test suite with MySQL. I guess we can merge this there, and then use that as target for any new work on MySQL compatibility.

[tompave]

I've amended the docs: a2e8e44

And I've created the mysql_support branch, that you can target with this PR to get it merged.

[tompave]

Update: I've given a stub at running the tests on MySQL. Currently a bit blocked locally because of this: xerions/mariaex#222

That issue also seems to suggest that mariaex is not compatible with MySQL 8.0. 🤔

[stewart]

Thanks for the README update. Based on the other discoveries in this thread, I think it's reasonable to use a mysql_support branch if this is a configuration you're interested in supporting in the future, and I've updated this PR to target it, as requested.

I am reasonably confident we're using a 5.7.20+ release of MySQL, but will verify for certain when I'm able.

[tompave]

Ok, I've had to downgrade my local env from MySQL 8.0 to 5.7, and now Mariaex can connect.
I've already done some work in the mysql_support branch to make it work with MySQL. If you want to check it out, you can use bin/console_ecto mysql to start a iex console connected to MySQL.

I'll now merge this and add some travis tasks for MySQL specifically.