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

MySQL support #42

Merged
merged 16 commits into from
Apr 13, 2019
Merged

MySQL support #42

merged 16 commits into from
Apr 13, 2019

Conversation

tompave
Copy link
Owner

@tompave tompave commented Apr 2, 2019

This branch tracks the progress to add support for MySQL to the Ecto persistence adapter, which currently only works with PostgreSQL.

The current status is that this branch contains some changes to work with MySQL on local dev and test, as well as on CI.

The builds still fail with MySQL because of how the table is locked using the Postgres syntax IN SHARE ROW EXCLUSIVE MODE, which MySQL doesn't understand. MySQL also seems to have different guarantees in terms of table locks within transactions (docs), which makes adapting the logic not straightforward. (solved)

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, but I don't have any concrete plan at the moment.

Thank you @stewart for starting this work by raising issue #40 and working on the first fix, with PR #41.

stewart and others added 7 commits March 27, 2019 12:16
ecto_sql supports UPSERT behaviour 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.
Ecto Persistence: Customize UPSERT options for MySQL adapter
@tompave tompave changed the title MySQL support [help wanted] MySQL support Apr 4, 2019
@tompave
Copy link
Owner Author

tompave commented Apr 4, 2019

Hi @stewart, I think that this is now working. Do you want to give it a spin?

@stewart
Copy link
Contributor

stewart commented Apr 4, 2019

Thanks @tompave - this changeset works as expected in my development environment, using FunWithFlags.UI and the console to test creating/updating various types of flags.

I'll be able to confirm more concretely once we've got it deployed to a staging environment for further testing, but I don't see any immediate problems.

@tompave
Copy link
Owner Author

tompave commented Apr 4, 2019

Ok. Leaving this here for the moment till you can confirm it works in a deployed env.

@tompave
Copy link
Owner Author

tompave commented Apr 10, 2019

Hi @stewart, any update on this?

@stewart
Copy link
Contributor

stewart commented Apr 10, 2019

I apologize, but we're still waiting for the go-ahead for a staging deploy, which should happen tomorrow. In local development, my team has not seen any issues with this branch.

I also noted the most recent release of ecto_sql includes a new MySQL adapter (as they're looking to stop using mariaex in the future), Ecto.Adapters.MyXQL. It's likely worth adding this to the check in db_type/0.

@stewart
Copy link
Contributor

stewart commented Apr 11, 2019

Still haven't gotten a green-light for our planned staging deploy, unfortunately - it's currently scheduled for tomorrow.

In the interest of keeping this PR moving, I've manually verified the MySQL persistence strategy for both the boolean and percentage-gate flags works as expected on both MySQL 5.7.25 and MariaDB 10.3.14, using both the mariaex and myxql Ecto adapters. Behaviour under load was tested by spawning a swarm of 100-500 Tasks that all attempted to set the same flag, and I didn't experience any deadlocks in this testing.

I additionally attempted to test with MySQL 8.0 and myxql, but was blocked by authorization issues that would require additional SSL plumbing to correct:

14:34:58.358 [error] GenServer #PID<0.312.0> terminating
** (MyXQL.Error) (2061) (CR_AUTH_PLUGIN_ERR) Authentication plugin 'caching_sha2_password' reported error: Authentication requires secure connection

Everything looks good to me.

@tompave
Copy link
Owner Author

tompave commented Apr 11, 2019

Thanks for running those tests. 👍

@stewart
Copy link
Contributor

stewart commented Apr 12, 2019

Can also confirm this branch is behaving as expected in our staging environments. Thank you for the additional MySQL support changes, and your patience with our slow turn-around for testing them.

@tompave tompave merged commit 248d843 into master Apr 13, 2019
@tompave tompave deleted the mysql_support branch April 13, 2019 11:13
@tompave
Copy link
Owner Author

tompave commented Apr 13, 2019

Cool, thanks for confirming it. 👍

This is now on master. Before releasing a new version, I want to add some more documentation and changelog entries, and I want to verify that everything still works with both ecto 3.0 and 3.1, and with both mariaex and myxql.

@tompave
Copy link
Owner Author

tompave commented Apr 13, 2019

Ok, released on Hex.pm as v1.3.0

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

Successfully merging this pull request may close these issues.

2 participants