WIP: Replace MSSQL dialect with Tedious.js implementation #2857

tylerjwatson · 2018-10-13T11:54:43Z

Tedious.JS MSSQL dialect driver

Hello Knex,

I have been working on deprecating the use of node-mssql in favour of tedious which is the underlying TDS stream driver for talking with Microsoft SQL server for about six months now. I wanted to take it into production before I made people aware of this patch to make sure it was worth submitting.

Rationale

I ran into major issues with every ORM or query library that uses node-mssql under the hood for MSSQL, which ultimately completely prevented me from running my system in production.

In a nutshell, the problem is that node-mssql is in itself a high-level driver that contains its own mechanisms which Knex either doesn't need, or conflicts with. To summarize the problems with using this library for this dialect:

Timeout acquiring resource messages, killing the application,
Random, huge latency spikes between query and result, making performance unstable,
Connection timeouts on startup, and
Inability to recover from error conditions

The SQL dialect experience with Microsoft SQL Server on node.js, and Knex in particular, is very poor because of this driver.

Patch details

This patchset contains a working implementation of tedious backed by a few months of my internal product running in production without issues. The patch discards the second-level connection pool provided by node-mssql and simply attaches Tedious connection objects to Knex's pooling mechanism like every other dialect.

With my driver running in production there have been no resource timeouts, no connection issues, and Knex receives the results of the query as fast as Tedious.js can deserialize the token stream. This implementation has been the most stable my project has ever been on MSSQL.

I don't use some of MSSQL's functions so I am unable to complete some of the driver, but it is good enough for me to roll into production right now.

Thanks for looking! 👍

it is not able to run two migrations in parallel when transactions are disabled it rejects setting foreign key where tableName is not typeof === 'string'

mssql: update driverName to be tedious meta: update lib builds to reflect new src

mssql: tablecompiler now has varchar(max) for json columns

Fixed an issue where rollback() would attempt to rollback a transaction that has not been opened yet. Calling beginTransaction() will not open the transaction until the first query, and if the first query has not executed yet, rollback() will attempt to roll back a transaction that is not open. mssql: do not issue rollback if connection is not in transaction

mssql: Fixed 'err is undefined' message on rollback when transactions haven't been commited yet

elhigu · 2018-10-13T12:20:42Z

Could you cleanup those extra generated files from PR, so that there willbe only relevant changes shown. And thank you very much for this. Changing implementation to use tedious directly will greatly help stability of mssql dialect of knex 👍

tylerjwatson · 2018-10-13T13:07:01Z

Crap! Totally forgot those were in there! I will clean ‘m up ASAP.

kibertoad · 2018-10-15T09:53:31Z

.travis.yml

@@ -10,9 +10,9 @@ cache:
 matrix:
  include:
  - node_js: "8"
-    env: TEST_ORACLEDB=true DB="maria mysql mysql2 postgres sqlite3 oracledb" CXX=g++-4.8 KNEX_TEST_TIMEOUT=60000 ORACLE_HOME=/u01/app/oracle/product/11.2.0/xe ORACLE_SID=XE OCI_LIB_DIR=/u01/app/oracle/product/11.2.0/xe/lib LD_LIBRARY_PATH=/u01/app/oracle/product/11.2.0/xe/lib
+    env: TEST_ORACLEDB=true DB="maria mysql mysql2 postgres sqlite3 oracledb" CXX=g++-4.8 KNEX_TEST_TIMEOUT=60000 ORACLE_HOME=/u01/app/oracle/product/11.2.0/xe ORACLE_SID=XE OCI_LIB_DIR=/u01/app/oracle/product/11.2../../lib LD_LIBRARY_PATH=/u01/app/oracle/product/11.2../../lib


Any particular reason for this change?

I don't remember making this change, no. It's possible that this diff is against a master which was changed elsewhere

kibertoad · 2018-10-16T08:49:11Z

@tylerjwatson I would like to contribute to the effort (e. g. sync with master), may I ask for a write permissions on your fork?

tylerjwatson · 2018-10-16T22:46:08Z

@kibertoad Can do, I just need to be careful for the time being.

The reason for the lib trash on this branch is because I needed to point yarn somewhere to the built sources so I can use it in production, and this seemed the least path of resistance to "get it working".

I'll ship my internal software onto a separate branch away from this pull, clean up the lib trash, and we'll take it from there.

Stand by.

knex: remove lib trash from repo knex: restore .travis.yml from master

meta: restore .gitignore from master meta: restore scripts/oracle-Dockerfile from master meta: restore package.json from master

tylerjwatson · 2018-10-18T10:40:48Z

Alright everyone, some cleanup has been done. The lib trash has been removed and some meta files that I didn't mean to touch have been checked out from master to bring them back to where they should be.

Some things I'll need some help with now:

Configuration format, I just jammed Knex's connection configuration straight into tedious, this is probably not what we want,
For the life of me I could never get npm run tape to work on plaintest, so I just removed it to develop the driver.
Transaction stuff could use a lookover
Migration tests aren't too happy

Run the tests with DEBUG=knex:mssql to see the driver in action. Hopefully now we can get some review happening 👍

kibertoad · 2018-10-18T10:52:37Z

I just jammed Knex's connection configuration straight into tedious

Isn't that what knex does for other dialects, though? I think {connection: {}} branch is always passed to driver as-is.

Can I get write access to your repo so that I could start fixing stuff, or you still need to remove some things?

kibertoad · 2018-10-18T10:54:05Z

package.json

@@ -54,6 +53,7 @@
    "sqlite3": "^4.0.0",
    "tap-spec": "^4.1.1",
    "tape": "^4.9.0",
+	"tedious":"^2.6.1",


Is indentation correct?

tylerjwatson · 2018-10-18T20:14:49Z

Done mate, knew there was something I forgot. :P

dhensby · 2019-03-08T23:40:46Z

src/dialects/mssql/index.js

+    debug("connection::destroy");
+    return new Promise((resolve, reject) => {
+      connection.once("end", () => {
+        resolve();


You need to be careful to make sure that if the connection has been closed due to a connection error, that this promise still resolves. because the end event won't trigger when calling connection.close() if it's already closed.

We've had this problem with the mssql lib (see https://github.com/tediousjs/node-mssql/blob/v5.0.0/lib/tedious.js#L278-L298)

kibertoad · 2019-03-09T10:47:07Z

@dhensby Do you think that there is still value in this PR? Most connection management issues with node-mssql were resolved in v5 already, and it is likely that v6 will address the remaining ones.

kibertoad · 2021-02-08T06:50:12Z

This is great, thank you!
Note: while I was syncing with master, I dropped few of previous changes from your branch, to focus on driver change per se, without changing the behaviour.
Notably: passing query context to errors; JSON support; removing explicit select rowcount from queries. Since I assume that you had good reasons for these changes, and you would need them should you return upstream, maybe we could discuss them and restore some or all of them in a separate PR?

tylerjwatson · 2021-02-08T07:08:24Z

Awesome! Glad to see this has finally landed after all this time. I am very excited about coming back upstream, it's never nice being left in the dark with new features. I'm particularly excited about the deprecation of Bluebird for native promises.

We're finding that when an SQL query error is generated we have no idea where it came from. Passing express req objects as query context is a small-overhead way to attach meaning to errors in the callbacks. Since query errors are logically owned by queries which have a query context we found no harm in sinking the query context into error emits. We can simply log them to the logger attached to the express request and we can get meaningful information like requesting user, response time, etc.

There's little use in the query-error event otherwise as there's nothing for our devops team to relate back to. This change is important and we'll need it before we return upstream unless I can figure out something else.

I can't remember what JSON support entails.
Removing @@rowcount was erroneous functionality and can be left alone as-is.

kibertoad · 2021-02-08T07:27:07Z

@tylerjwatson Gotcha, I'll make a PR restoring query context and JSON support shortly for your reviewn then.
Another thing missing are the TS type changes. Could you look into syncing current knex mssql config types with what is currently supported?

tylerjwatson · 2021-02-08T07:50:13Z

@kibertoad Nah let's do it properly in separate PRs.

For JSON support see #4278,

I'll look at doing query-error support properly shortly

I'll also update the types appropriately

tylerjwatson added 11 commits June 7, 2018 19:24

wip: translating node-mssql to tedious

c3b18e1

acquireRawConnection

a79ad48

All tests pass apart from two

fa51a25

it is not able to run two migrations in parallel when transactions are disabled it rejects setting foreign key where tableName is not typeof === 'string'

Fixed an issue where the error handler would throw exceptions

270079c

meta: adding Lib to git for the time being

6f4e595

Production build updates

bc0f216

mssql: update driverName to be tedious meta: update lib builds to reflect new src

Changed JSON type from 'text' to 'varchar' in MSSQL

6f79c5f

mssql: tablecompiler now has varchar(max) for json columns

Adding lib to builds

2b7387a

Merge branch 'tedious' of github.com:tylerjwatson/knex into tedious

c5772cc

Fix bug in rollback

041230c

mssql: Fixed 'err is undefined' message on rollback when transactions haven't been commited yet

tylerjwatson changed the title ~~WIP: Replace MSSQL dialect~~ WIP: Replace MSSQL dialect with Tedious.js implementation Oct 13, 2018

kibertoad mentioned this pull request Oct 15, 2018

Fix linter errors in tests #2859

Merged

kibertoad reviewed Oct 15, 2018

View reviewed changes

kibertoad added improvement MSSQL labels Oct 16, 2018

tylerjwatson added 2 commits October 18, 2018 20:28

Cleanup for PR

0ff9055

knex: remove lib trash from repo knex: restore .travis.yml from master

Updates for PR

eb4635b

meta: restore .gitignore from master meta: restore scripts/oracle-Dockerfile from master meta: restore package.json from master

kibertoad reviewed Oct 18, 2018

View reviewed changes

meta: upgrade tedious@3

4e90239

tylerjwatson mentioned this pull request Feb 5, 2019

RFC: Add .raw support to Builder instances analogous to .whereRaw et al. #3032

Open

dhensby reviewed Mar 8, 2019

View reviewed changes

kibertoad mentioned this pull request Feb 8, 2021

fix mssql remove unused pool config option #4235

Closed

nickrum mentioned this pull request Feb 12, 2021

Throw error if the array passed to insert is empty #4289

Merged

dhensby mentioned this pull request Nov 7, 2023

Use Datetime2 as default type for javascript Date objects tediousjs/node-mssql#1578

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Replace MSSQL dialect with Tedious.js implementation #2857

WIP: Replace MSSQL dialect with Tedious.js implementation #2857

tylerjwatson commented Oct 13, 2018 •

edited by kibertoad

elhigu commented Oct 13, 2018

tylerjwatson commented Oct 13, 2018 via email

kibertoad Oct 15, 2018

tylerjwatson Oct 16, 2018

kibertoad commented Oct 16, 2018

tylerjwatson commented Oct 16, 2018

tylerjwatson commented Oct 18, 2018 •

edited

kibertoad commented Oct 18, 2018

kibertoad Oct 18, 2018

tylerjwatson commented Oct 18, 2018

dhensby Mar 8, 2019

kibertoad commented Mar 9, 2019

kibertoad commented Feb 8, 2021

tylerjwatson commented Feb 8, 2021

kibertoad commented Feb 8, 2021

tylerjwatson commented Feb 8, 2021

WIP: Replace MSSQL dialect with Tedious.js implementation #2857

WIP: Replace MSSQL dialect with Tedious.js implementation #2857

Conversation

tylerjwatson commented Oct 13, 2018 • edited by kibertoad

Tedious.JS MSSQL dialect driver

Rationale

Patch details

elhigu commented Oct 13, 2018

tylerjwatson commented Oct 13, 2018 via email

kibertoad Oct 15, 2018

Choose a reason for hiding this comment

tylerjwatson Oct 16, 2018

Choose a reason for hiding this comment

kibertoad commented Oct 16, 2018

tylerjwatson commented Oct 16, 2018

tylerjwatson commented Oct 18, 2018 • edited

kibertoad commented Oct 18, 2018

kibertoad Oct 18, 2018

Choose a reason for hiding this comment

tylerjwatson commented Oct 18, 2018

dhensby Mar 8, 2019

Choose a reason for hiding this comment

kibertoad commented Mar 9, 2019

kibertoad commented Feb 8, 2021

tylerjwatson commented Feb 8, 2021

kibertoad commented Feb 8, 2021

tylerjwatson commented Feb 8, 2021

tylerjwatson commented Oct 13, 2018 •

edited by kibertoad

tylerjwatson commented Oct 18, 2018 •

edited