This PR adds support for .transaction()

[mdierolf]

New pull request for the functionality in this pull request: https://github.com/sidorares/node-mysql2/pull/811

I had to create a new PR and close the old one, as github will not allow me to change the head branch once a PR is filed

This adds support for PromiseConnection.transaction() to MySQL2

[sidorares]

thoughts re new api @sushantdhiman @gajus @mscdex @dougwilson ? In general I try to resist adding new surface api here but it looks quite useful for end user

https://github.com/CloudQuote/node-mysql2/blob/b3073f992512459eb06c053e75d49dfdb42f0f69/README.md#using-managed-transaction

[mscdex]

I don't have an opinion as I don't use Promises.

[mdierolf]

One thing to note:

The proposed .transaction() function includes options that the current .beginTransaction() function does not currently accept.

IMO, if .transaction() is going to accept options, then .beginTransaction() should probably do so as well.

[gajus]

I have been using an interchangeable API without any issues for a while now (https://github.com/gajus/slonik#slonik-query-methods-transaction).

[gajus]

I would argue that beginTransaction() should be removed altogether in favour of transaction().

[sidorares]

I would argue that beginTransaction() should be removed altogether in favour of transaction().

I don't think it's an option, we have to account for mysql2 users who currently use beginTransaction and mysqljs/mysql users whu might occasionaly switch between libs. If we decide to needs to be coordinated between both and go through long deprecation cycle. Much easier for promise wrapper though

[sushantdhiman]

Looks good, will need some integration tests and it should be good to go

[dougwilson]

Looking at the user docs for the API I think it is a really neat use for promises.

As for this vs beginTransaction, I guess the only comment I would have is that the new API does not support callbacks at all while the old one does. If the old is removed that would be the only consideration: should callbacks just be removed from the entire driver surface?

[sidorares]

@dougwilson you mean remove .beginTransaction() from both promise wrapper and standard api? This is harder as I think 'standard' .beginTransaction() is used actively

[dougwilson]

Sorry, I meant that if the idea is after landing this new API that beginTransaction would end up deprecated and removed, then there wouldn't be a transaction callback API any longer.

[mdierolf]

I'd recommend leaving beginTransaction() for compatibility with mysqljs, and adding the following behavior: -Calling beginTransaction() will mark the Connection as having an open transaction -Calling commit() or rollback() will remove this mark from the Connection -When a connection is released to the Pool, if it is marked as having an open transaction, rollback() will be called -Remove the rollback-on-failure logic from PromisePool.transaction() and rely on the pool to automatically rollback on release

This model will allow you to have managed transactions when using the callback API, and managed transactions + automatic connection release when using the Promise API

[mdierolf]

Any thoughts on this PR? I've been using it in production for a while, and updated it yesterday to be compatible with the new class-style code that is now in promise.js, which seems to work fine

I'd like it if we could make a decision one way or another; if it's not something that should be included in the main tree i'd rather convert it into a monkey patch in a separate repository so I can maintain it without having to merge changes into my fork

Also, any feedback as far as automatically rolling back open transactions when a connection is returned to the pool? If some bad code throws an exception, and then returns a connection to the pool that has an already open transaction, it seems like that would be very bad situation.

[henricavalcante]

I'm really looking forward to be able to use transactions on node-mysql2, if there is anything I can help here I'll be glad to do.

[mdierolf]

So we've been using this for the past year, it helps a little bit in simplifying our code, but I am thinking of re-designing this, as it doesn't really solve the problem we envisioned.

We have a collection of related problems in the code we have which uses node-mysql2:

1. We need a way to run one or more queries on the same connection - sometimes we use this API but other times we concatenate the queries and run them with "multipleStatements" option, which is kind of clunky to use since we have to put all our SQL into one long string.
2. We are typically constructing WHERE strings and replacement values in a rather clunky fashion using two arrays we push into, which gets especially weird when using multipleStatements since the replacements do not correspond to a particular statements, so it is easy to pass values to the wrong statement if you do not construct the placeholders correctly.
3. We need a more automated way of retrying failed queries (either because we were handed a dead connection from the pool, because the connection died in the middle of the sequence of queries, or because the transaction rolled back due to conflict)
4. We need a way of routing read vs write queries to different SQL interface for scaling.

So to fix these 4 issues, i'm thinking of making a new module which implements a "managed" query interface that could run on top of the mysql or mysql2 driver - which would allow you to define a chain of queries, possibly with intermediate code run between them to do calculations, which would then run them either in a transaction or raw.

It could also flag the individual queries as read vs write so they can be routed to one or more pools, and would handle failures more intelligently - for example you might choose to retry queries twice then fail hard.

The idea being to add on a couple key convenience features that ORM systems implement while still writing raw SQL queries.

As such, i'm not sure if it makes a ton of sense to merge in one particular method of working with transactions into the MySQL driver - the API in the driver should probably be unopinitated (and allow you to do everything) - or opinionated but optional (like what I am thinking of)