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

Reduce amount of prepared statements #243

Merged
merged 7 commits into from
Dec 12, 2018
Merged

Conversation

archan937
Copy link
Contributor

@archan937 archan937 commented Oct 25, 2018

This is similar to Postgrex (0.14.0-rc.1) so the prepare option is either :named (default) or :unnamed in which :unnamed forces the name of the prepared statement to be "".

The corresponding Travis CI build: https://travis-ci.org/bettyblocks/mariaex/builds/448283254
This pull request should comply to: #182 (comment)

Use the `GRANT` command (instead of `CREATE USER`) to create the `mariaex_user`
user if the account does not exist already

(works from MySQL 5.5 and above)
This is similar to Postgrex (0.14.0-rc.1) so the prepare option is either
`:named` (default) or `:unnamed` in which `:unnamed` forces the name of
the prepared statement to be `"_unnamed_"`.
@coveralls
Copy link

Coverage Status

Coverage increased (+1.9%) to 88.172% when pulling 1d7173f on bettyblocks:master into b5f7a0d on xerions:master.

@coveralls
Copy link

coveralls commented Oct 25, 2018

Coverage Status

Coverage increased (+2.0%) to 88.256% when pulling cfe38ae on bettyblocks:master into b5f7a0d on xerions:master.

Copy link
Contributor

@fishcakez fishcakez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we remove changes unrelated to the main change? If you want to add them please do in separate PR.

@@ -335,6 +348,7 @@ defmodule Mariaex.Protocol do
end
end
def handle_prepare(%Query{type: :binary} = query, opts, %{binary_as: binary_as} = s) do
query = sanitize_query(query, s)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be done in prepare_insert

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, the code is not that easy to follow in order for me to fix the issue by altering just prepare_insert. I cannot quite get along with both LruCache and Cache to get the desired behaviour.

Can you give me some pointers maybe?

Copy link
Contributor Author

@archan937 archan937 Oct 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, @fishcakez . I have cleaned up some unnecessary code. I am deleting the LRU cache when prepare == :unnamed which solved the problem of not being able to use "" as unnamed query name.

The final problem: I cannot figure out how to stabilize the prepare statements count by moving the query override from handle_prepare to prepare_insert. I guess this is as far as I can go regarding complying to your feedback.

Hopefully you can help me out on this.

@archan937
Copy link
Contributor Author

archan937 commented Oct 26, 2018

About the changes in the test setup, I will put them in a separate PR when this is resolved

@archan937 archan937 changed the title Reduce amount of prepared statements and improve test database setup Reduce amount of prepared statements Oct 30, 2018
@archan937
Copy link
Contributor Author

I reverted the irrelevant changes.

@archan937
Copy link
Contributor Author

Hey, @fishcakez. Can you explain what needs to be done any further to get this PR follow through?

@liveforeverx liveforeverx merged commit 5f4aa50 into xerions:master Dec 12, 2018
@liveforeverx
Copy link
Member

Thank you!

@archan937
Copy link
Contributor Author

You're welcome! ;)

@fishcakez
Copy link
Contributor

This change confuses unnamed and lru cache functionality. Unnamed should move to using the lru cache, and disabling the LRU cache should be a second option (whereby unnamed queries get closed once finished with). While it has the intended end result I think we should have split this change in two. Hopefully we can fix it before the next release.

@archan937
Copy link
Contributor Author

Well, I was well aware that I was not supposed to "confuse" the unnamed and LRU cache functionality. You gave that as feedback on the PR and afterwards I tried to solve it complying to your pointers.

Unfortunately, I did NOT manage to fix the prepared statements leak that way. And because of that, I asked you for some assistance but you did not reply on my request (see: #243 (comment)).

Anyways, I hope you will be able to solve it without confusing the unnamed and LRU cache functionality. Looking forward to your reply.

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.

None yet

4 participants