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

Prepared statements via .execute() being cached indefinitely #393

Closed
Twipped opened this issue Sep 8, 2016 · 5 comments
Closed

Prepared statements via .execute() being cached indefinitely #393

Twipped opened this issue Sep 8, 2016 · 5 comments

Comments

@Twipped
Copy link
Contributor

Twipped commented Sep 8, 2016

We ran into a problem this morning where we suddenly started getting a whole lot of Can't create more than max_prepared_stmt_count statements errors from our service. After some digging we came to discover that mysql2's .execute() function doesn't ever close the statements that it prepares. They get stored on the open connection's statement cache and never get purged unless the connection closes.

We have been running all queries through .execute(), including those with dynamic ranges of arguments, so we hit our DB's max max_prepared_stmt_count limit fairly quickly.

We're now making changes to our code to limit when we use .execute(), but it would be nice to have a setting to expire cached queries after a set amount of time.

Additionally, the documentation could be clearer about the implications of using .execute() vs preparing the statements yourself.

@sidorares
Copy link
Owner

sidorares commented Sep 8, 2016

I'm completely in favour about adding more documentation about prepared statement lifecycle on server and client

The reason I have not yet added auto-closing of unused statements is that often people misuse them in a execute('select * from foo where bar > ' + i) way. What's the reason you have dynamic sql code in execute? Also, unlike max_connections, max_prepared_stmt_count is relatively easy to bump and it can be set to a relatively high value without big impact to server cpu or memory. Documentation is saying about "max_prepared_stmt_count": "The default value is 16,382. The permissible range of values is from 0 to 1 million"

Another complexity is that statement close() command at the protocol level does not send success or error notification, it's one way message. Also I think max_prepared_stmt_count is per server, and LRU with some limit would cap connection (though if set to max_prepared_stmt_count/max_connections limit this might work)

I think it would be good to have LRU instead of letting them live until connection is closed, with expiration based on LRU size, not max idle time (e.i if limit is 100 when stmt 101 is prepared the first one from cache is closed). Note that due to the way commands are queued in the connection the state of the queue would look like "prepare stmt 101, execute stmt 101, close stmt 1" after execute call (upd. actually this is not a problem. execute command can insert close into queue first before adding prepare/execute)

@Twipped
Copy link
Contributor Author

Twipped commented Sep 8, 2016

What's the reason you have dynamic sql code in execute?

As of today we don't, we've now changed all those queries over to using query(). They're all cases where we have a variable number of bound parameters due to id lists and the like. We didn't realize the significance of doing so until it stopped working.

An LRU sounds like a good plan. That would at least save others from making the same mistake we did. max_prepared_stmt_count is easy to change if you control your database server, but not everyone does.

@sidorares
Copy link
Owner

sidorares commented Sep 8, 2016

Executing and unpreparing is probably slower than doing query() so I think prepared statements are meant to be used only for non-dynamic queries where you have fixed query and separate set of (fixed number) input parameters

max_prepared_stmt_count is easy to change if you control your database server, but not everyone does.

if permission allow you can do something like this:

connection.query('SET GLOBAL max_prepared_stmt_count=10', function (err) {

@sidorares
Copy link
Owner

@ChiperSoft LRU cache for statements landed in master and going to be published as 1.1.0 soon

@Twipped
Copy link
Contributor Author

Twipped commented Sep 19, 2016

Awesome, thanks!

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

No branches or pull requests

2 participants