node-mysql2
node-mysql2 copied to clipboard
sidorares
What happens when a connection is released?
As far as I can understand a pool connection when released will at least make it available to any waiting connection. This means it's a persistent connection.
Does this perform any kind of cleanup? What happens if it is released with a transaction still in play?
Is it possible to have a pool that instead keeps connections open in advance rather than reusing?
this is the code for releaseConnection - https://github.com/sidorares/node-mysql2/blob/2824df36a3fb7113ae44320871ae574ca56f61a7/lib/pool.js#L79-L94
In short, it's just returned to list of "free" connection and is up for grabs for next .getConnection call. You are responsible for cleaning up connection state.
If I destroy the connection instead such as using end does the pool know to create a new connection?
For transactional workloads first and foremost I usually have the connection die on any error at all with the default for a lost connection always being rollback. I tend to assume the worst rather than risk committing a partially complete transaction.
This also applies to connection persistence in those cases. Unless a clean state can be guaranteed I wouldn't use it. I already saw at that code and honestly had to ask because it's extremely unsafe that I couldn't believe my eyes and hoped something else was happening out of sight.
MySQL transactions offer various guarantees and I feel like the documentation where possible should be very clear about whether or not it maintains those.
The simplest way to be able to maintain them is if rather than release the connection can be ended and the pool will make a new one. I have a feeling there may be a command or will be to reset the current connection but I'm not sure.
I would not recommend implementing an attempt to clean connections by hand because it gets very complicated very quickly with transaction state, variable state, temp table state, etc.
I believe traditionally this is meant for reusing connections safely in a pool:
https://dev.mysql.com/doc/refman/8.0/en/mysql-reset-connection.html
https://github.com/mysql/mysql-server/blob/91a17cedb1ee880fe7915fb14cfd74c04e8d6588/libmysql/libmysql.cc
In the mean time a work around is crucial or use of pool is a no-no for guarantee based dev unless DIY.
I think ensuring and documenting end is fine to use instead of release for safety is crucial.
Aside from that full transaction support is really complex. I've never seen an application library do it properly.
Start transaction has to take read only, write, isolation, etc then for nested transactions you have to check that, return a transaction handler (people use counters but it doesn't detect a missed call to commit, etc) and then normally nested transactions simply skip starting the transaction as already, commit is easily ignored and you go down the stack but rollback raises questions. Then you have savepoints, chain, etc. Gets more complicated if abstracted to the point start transaction returns a new exclusive connection, or not if only user (but not possible here on pool level with no ref counting if command is directly on pool so would always be new connection).
I'm sceptical that it's ever been done.
Though getting the basics would at least allow people to reliable add what they need on top.
changeUser() with no parameters resets connection state
https://github.com/mysqljs/mysql#switching-users-and-altering-connection-state
The problem I see here is that it adds big performance penalty - chageUser command itsef is one extra network trip, plus prepares statements are invalidated, plus query cache etc etc. Maybe we should have pool config parameter "reset connection state after reuse", is set to true connection automatically gets changeUser command before release
similar issue: #934
This one can be a bit opinionated sometimes but is important. People tend to operate MySQL with two philosophies, performance first or data safety first and usually they're mixed up.
I'm going for safety first so replacing release with end connection for exclusive connections and hopefully that'll work.
end() { const err = new Error( 'Calling conn.end() to release a pooled connection is ' + 'deprecated. In next version calling conn.end() will be ' + 'restored to default conn.end() behavior. Use ' + 'conn.release() instead.' ); this.emit('warn', err); // eslint-disable-next-line no-console console.warn(err.message); this.release(); }
Almost went down the wrong path. It seems destroy is the only canonical way and it's not async though worser things have happened. At a quick glance it looks as though the pool should refill the connection?
Down the line being able to reset the session instead as a minor performance boost over a full reconnect is a nice to have with safety first being important.
Although changeUser appears to be public it doesn't look like something the pool is aware off. It would technically work though also having an explicit reset connection command would be more concise.
Really the pool connection and pool would in an ideal world have a better state tracking and matching mechanism. I think it would be very little different to this:
https://github.com/sidorares/node-mysql2/blob/a0ea513b0d4929e6ee61639a09f38a85ac01f7fa/lib/commands/ping.js
For now I'm trying with making a custom reset command (same as ping but 0x1f) then going to try addCommand on the connection but it leaves a lot to be desired such as if an error in logic somehow causes a problem while they're still a command in waiting it's unclear if the connection will hang or not or worse.
It gets called as part of my own connection wrapper's destroy which is supposed to always be called along with my own half baked GC conventions.
https://dev.mysql.com/doc/internals/en/com-reset-connection.html
I definitely want to see .reset() implemented here ( and maybe help mysqljs/mysql implement it )
Few things in addition just having reset() { this.addCommand(new Commands.Reset() }:
- it should probably reset prepared statements cache (but likely not reset parsers cache)
- promise wrapper (AFAIK would be added automatically but need to double check)
- unit tests
- consider adding it to pool release ( maybe only when config flag
resetOnReleaseis set for backwards compatibility )
also need to add command code to https://github.com/sidorares/node-mysql2/blob/master/lib/constants/commands.js
I think it's also worth mentioning the functionality requested here can also be related with: https://github.com/sidorares/node-mysql2/issues/586#issuecomment-307376690
Use cases
- If people want performance they are likely to want to switch to Transaction Isolation Levels - READ COMMITTED. Sometimes this needs to be done at the connection level, rather than the server level.
- If people want data safety first, the connections should be cleaned up
Issues
- You can not set Transaction Isolation Levels - READ COMMITTED with async code in the current event system.
- You can not call mysql_reset_connection() with async code in the current event system.
Recommended actions
- Document the events, which I assume are similar to mysqljs - https://github.com/mysqljs/mysql#acquire
- Change the events so they can handle callbacks when the extra code has finished running (aka support promises)
- Update documentation to include two new headings of "Optimizing for performance" and "Optimizing for data safety" which can document each of these scenarios
