Bulk Insert

[bretcope]

Since it's a significant new feature which I didn't do via pull request, I just wanted to open an issue here for a few days to announce that bulk insert is now implemented.

I added experimental support a couple weeks ago, and I believe we've worked out all the kinks since then, so tonight I wrote a test, documented it, and declared the API stable. It's published as 1.4.0.

I've added it to Tedium, which is the generator-based wrapper I've been working on. @patriksimek if it seems within the scope of node-mssql, again, the API should be stable now, so feel free.

If there are any questions, please add them to this thread.

[patriksimek]

Thanks Bret, I have found a strange behaviour. Consider this test:

exports.bulkLoadError = (test) ->
  config = getConfig()

  connection = new Connection(config)

  connection.on('connect', (err) ->
    test.ifError(err)

    bulk = connection.newBulkLoad('#tmpTestTable2', (err, rowCount) ->
      console.log err, rowCount

      test.ifError(err)

      test.strictEqual(rowCount, 0, 'Incorrect number of rows inserted.');

      # temporary disabled to let ECONNRESET error to be thrown
      #connection.close()
    )

    bulk.addColumn('x', TYPES.Int, { nullable: false })
    bulk.addColumn('y', TYPES.Int, { nullable: false })

    # create temporary table
    request = new Request("CREATE TABLE #tmpTestTable2 ([id] int)", (err) ->
      test.ifError(err)

      bulk.addRow({ x: 1, y: 1 })

      connection.execBulkLoad(bulk)
    )

    connection.execSqlBatch(request)
  )

  connection.on('end', (info) ->
    test.done()
  )

  connection.on('error', (err) ->
    console.error("ERROR: #{err.stack}")
  )

  connection.on('debug', (text) ->
    console.log(text)
  )

As you can see, I'm trying to insert data to table that has a different structure of columns. I would exepect an error here, but strange things happen. First of all the connection hangs up immediately once you send the BULK_LOAD payload with socket error ConnectionError: Failed to connect to <ip>:1433 - read ECONNRESET. What debug log says:

Sent
  type:0x01(SQL_BATCH), status:0x01(EOM), length:0x005E, spid:0x0000, packetId:0x01, window:0x00
  0000  69006E00 73006500 72007400 20006200 75006C00 6B002000 23007400 6D007000  i.n.s.e. r.t. .b. u.l.k. . #.t.m.p.
  0020  54006500 73007400 54006100 62006C00 65002800 5B007800 5D002000 69006E00  T.e.s.t. T.a.b.l. e.(.[.x. ]. .i.n.
  0040  74002C00 20005B00 79005D00 20006900 6E007400 2900  t.,. .[. y.]. .i. n.t.).
  SQL Batch - insert bulk #tmpTestTable([x] int, [y] int)
State change: LoggedIn -> SentClientRequest

Received
  type:0x04(TABULAR_RESULT), status:0x01(EOM), length:0x0011, spid:0x003D, packetId:0x01, window:0x00
  0000  FD0200FD 00000000 00  ........ .
{ name: 'DONE',
  event: 'done',
  more: false,
  sqlError: true, # <--- notice this
  attention: false,
  serverError: false,
  rowCount: undefined,
  curCmd: 253 }
State change: SentClientRequest -> LoggedIn

What documentation 2.2.7.5 says:

0x2: DONE_ERROR. An error occurred on the current SQL statement. A preceding ERROR token SHOULD be sent when this bit is set.

But the problem is, there is no preceding ERROR token. If this is a normal behaviour, Tedious should always check for sqlError flag regardless if there was an ERROR token or not.

Before we start making a changes, I would like to ask you to check it. Thanks.

[bretcope]

I can definitely repro this, thank you. I'm thinking you're right that Tedious should always check the sqlError flag to be on the safe side. Since the documentation says it SHOULD, not it MUST, that implies it may, in some cases, not include an error token, and this appears to be one of those cases.

I suppose we need to decide what that error message should look like since it's essentially an unknown error.

I assume the best way to handle this would be to check for token.sqlError in the @tokenStreamParser.on('done') method. Something like

if token.sqlError && !@request.error
  @request.error = RequestError('An unknown error has occurred.', 'UNKNOWN')

Do we need to makeup a mock ERROR token and emit an errorMessage for consistency?

In certain contexts, like bulk load, we could re-write the message before sending it to the callback in order to give hints as to the likely cause, while still indicating that the true cause is unknown.

Thoughts?

[patriksimek]

I believe we don't need an errorMessage, there are plenty of errors passed directly to callback.

Your solution is fine by me.

[bretcope]

Okay, I'll try to get to it today or tomorrow.

[bretcope]

Fixed and published as 1.4.1. I added a slightly modified version of your test to check against.

[patriksimek]

👍 Thanks.

[rossipedia]

Any reason to leave this open, or tagged as pending release?

[bretcope]

I just wanted to give people a chance to look at it. It can be closed now.

[patriksimek]

@bretcope, any thoughts on that? tediousjs/node-mssql#81