port prepared statements support from node-mysql2

[sidorares]

This issue is to discuss public api and track progress of port of prepared statements (aka "binary protocol") implementation from https://github.com/sidorares/node-mysql2

See also: #274 and sidorares/node-mysql2#52

I'll start discussion in #274 again once there is something to try.

node-mysql2 api: exactly the same as query(), but it's execute()
Under the hood:

• sql query is used as a key in internal statement id's hash. If there is id, it is used.
• if there is no id in hash, COM_STMT_PREPARE command is executed and id saved.
• COM_STMT_EXECUTE command with id and serialised parameters is sent if prepare is successful
• no interpolation/escaping happens on the client. All parameters are sent individually using binary protocol.

Currently there is no separate public prepare and unprepare and I'm not sure if we need them (unprepare might be useful if you intend to create hundreds of statements, but most often than not this is sign of abusing the idea of prepared statements, for example trying to prepare query with already escaped parameter)

Both parameters and result values are sent "typed" (opposed to "text protocol" where they are serialised as text) so there might be slightly different semantics of typeCast.

[felixge]

SGTM. FWIW when I worked on this for a bit, I opted for a prepare() function that returns a prepared statement object: https://github.com/felixge/node-mysql/blob/4680963f1a9c75764ecb240896df5467e17a8d96/test/integration/connection/test-prepare.js#L26-29

I think I like this a little better than your hashing approach as it's less magically / gives us the ability to unprepare() if we need it. However, since I won't have time to contribute to the implementation, I'm happy to let you make the final decision on this.

[sidorares]

What if execute will be able to accept both string and number as a parameter? (query / ps id)
Same for "unprepare" - if string is passed then id from hash is used, otherwise number directly.

[sidorares]

Also, ideally I'd like to reuse as much as possible query integration tests with execute - having identical api is helpful

[felixge]

How about execute() using the prepare() API I've suggested under the hood?

[sidorares]

I tend to think that it adds a lot of unnecessary complexity: you need to store statement somewhere and most people will end up in similar implementation (e.i hash in connection object) so why not have it by default? The whole point of statements is to be reused, so it's not sql->prepare->execute->unprepare but rather sql->prepare in init and sql->lookup statement-> execute where you use it.

[felixge]

I'm not sure how people will use it. If I was to use it, I'd probably pass around a reference to the statement rather than relying on a shared lookup table. But since I'm not going to use this feature for now, I'll leave this up to those who want to use it.

@sidorares might be worth bringing this up in the other ticket you referenced as it will allow a few other people to get a notification for discussing this.

[thesmart]

+1 for the hashing proposal. I would rather not have to retain references to statements already created in some prior init-closure. This practice would just encourage more global variables. The hashing proposal proposed by @sidorares is fast and secure by default. There are a few opinions to add:

• each mysql instance should have its own prepared statements hash, not some static shared static hash
• prepare() and unprepare() should be available for completeness
• what about a flag for forcing re-preparation of a statement?

[thesmart]

Any plans to fork and provide this feature in node-mysql? @sidorares ?

[sidorares]

@thesmart I already started doing it in feature branch of this repo - https://github.com/felixge/node-mysql/tree/prepare-mysql2 . Stay tuned - should be ready soon :)

[sidorares]

what do you mean by "mysql instance"? It's per conection as server disposes statement id on disconnect (and it's local to connection - you can't prepare statement and use it's id in another connection)

Why would you need re-preparation? See #274 (comment) Statements prepared manually (with "prepare") are not stored in the hash and thus you are aloowed to unprepare/prepare again or have multiple prepared copies of the same statement

[noah-goodrich]

Is this still in active development? It doesn't appear that the branch or this thread has seen any activity in a while.

[sidorares]

It's not actually that much left to do, it's just I have lots of other things on the list. Hopefully will be able to get back soon, but if anyone wants to do it - don't be shy

[rdooh]

Can I assume that this is on hold until further notice? I'm in need of the feature asap, but contributing is really beyond my depth... so just to be clear, I'm not trying to stir anything up - all the work here, which has been generously shared, is epic in terms of effort and community impact. You guys are heros.

What I'm trying to ascertain is what my best route forward is, given that my projects will require PS's. I see that this was dropped from the 2.1 milestone... @sidorares, I quite like mysql2, but have some mild concerns about its longevity, given that the two have a great deal of overlap in terms of the role they serve, and you are also apparently contributing to mysql... I'd be more confident with mysql2 if it was at 1.0...

Aside from sounding the call to roll up our sleeves and pitch in, can anyone make interim recommendations for those of us that aren't equipped to contribute to this feature? My gut tells me that I should write my own wrapper api module and use mysql2 for the PS feature, so that I can integrate it into my projects now, and I can re-address the mysql/mysql2 decision again at a later time should things change.

[sidorares]

@rdooh I'm really keen to finally implement true PS here, it's just currently too much other projects on my list.
I'd say that biggest advantage of mysql vs mysql2 is having @dougwilson as maintainer :)

From what I've seen it might be relatively safe to use mysql2 in production, but my main focus is mainly on speed. Version number is just semver, it'll be 1.0 after breaking api change.

[rdooh]

@sidorares - many thanks for your assessments etc. And re: wish-lists vs workloads & reality, I totally get it. I've got my short-term plan together, and I look forward to watching things develop whenever you (and others) get a break from your other commitments. Kudos to you and everyone else involved.

[dougwilson]

So I, personally, would like to see the binary protocol supported here eventually, but personally, there are a few "big ticket" items that need to be addressed before I can really get to the binary protocol: 1) charsets are completely broken with this module and 2) the columns always come as keys to a row object (i.e. there is no array-of-array interface).

In the big scope of features, this is, really, my game plan:

1. Fix the encoding issues so there is no longer a possibility of storing/reading corrupt data.
2. Add array-of-array row retrieval
3. Add binary protocol (this ticket)

@rdooh I hope this helps, especially if just knowing binary protocol support is definitely wanted. And re: milestone change, that was because for a while the versions were semi-stagnant, but they are moving faster now, so keeping a milestone on this ticket doesn't make sense until it's basically being worked on so it doesn't have to keep getting bumped :)

[rdooh]

@dougwilson - this helps charting my own course forward immensely. I greatly appreciate your efforts, and the insights. Many thanks.