Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

SET defaults #83

Merged
merged 1 commit into from

3 participants

@kppullin

Updated the initial sql call to set default settings flags to match those generated by ADO.NET/SQL Server Management Studio, as validated by SQL Server Profiler.

These are considered "sane" & "least suprise" defaults (for example, linked server queries fail without a couple settings here, etc).

The same effect may be forced by the ODBC_ON flag in login7-payload.coffee, however I'm unaware if this can cause other unintended side effects (again, settings can be verified in SQL Server Profiler).

This also addresses issue #59

@kppullin kppullin SET defaults
Updated the initail sql call to set default settings flags to match
those generated by ADO.NET/SQL Server Management Studio.

These are considered "sane" & "least suprise" defaults (for example,
linked server queries fail without a couple settings here, etc).

This also addresses issue #59
c587a51
@pekim
Owner

Thanks. This looks pretty reasonable. I should be able to take a better look at the weekend, including running the integration tests.

We need to think about whether it's a good idea setting the transaction isolation level. This is already defaulted in the connection options (http://pekim.github.com/tedious/api-connection.html#function_newConnection), and I'm inclined to think that that's the way it should stay. Although perhaps the default there is wrong, and it should default to READ_COMMITTED?

@kppullin

Thanks for the response & feedback! If you've additional suggestions on adding tests or other changed I'd be happy to help : )

Regarding the isolation level, IMHO it should be set to READ_COMITTED instead of READ_UNCOMITTED.

Reasons being: 1) this is the default as shipped by Microsoft and adheres to the principle of least surprise (at least for those coming from a SQL Server/.NET ecosystem), and 2) on a personal level, I like to avoid dirty reads where possible, and only use READ_UNCOMITTED where I know and have accepted the risks.

Additionally, from my brief look at the code, the 'config.isolationLevel' setting is only applied for transactions. Items outside of the transaction use the default isolation level or SQL Server, which is READ_COMMITTED.

So while this would be a breaking change with respect to the documentation, the current implementation only adheres to the documented behavior when using explicit transactions.

For reference, here are the defaults applied in the 'Audit Login' profiler event when connecting with tedious outside of a transaction:

set quoted_identifier off
set arithabort off
set numeric_roundabort off
set ansi_warnings off
set ansi_padding off
set ansi_nulls off
set concat_null_yields_null off
set cursor_close_on_commit off
set implicit_transactions off
set language us_english
set dateformat mdy
set datefirst 7
set transaction isolation level read committed
@pekim pekim merged commit 94e9616 into from
@jonbuffington

@pekim Could you publish a 0.1.4 release that includes this change? Running into an issue that would be resolved by this PR.

@pekim
Owner
@momow momow referenced this pull request from a commit in momow/tedious
@bretcope bretcope Make pingdom complain if the cache is outdated. fixes #83 0350b0a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 12, 2013
  1. @kppullin

    SET defaults

    kppullin authored
    Updated the initail sql call to set default settings flags to match
    those generated by ADO.NET/SQL Server Management Studio.
    
    These are considered "sane" & "least suprise" defaults (for example,
    linked server queries fail without a couple settings here, etc).
    
    This also addresses issue #59
This page is out of date. Refresh to see the latest.
Showing with 16 additions and 1 deletion.
  1. +16 −1 src/connection.coffee
View
17 src/connection.coffee
@@ -408,7 +408,22 @@ class Connection extends EventEmitter
@tokenStreamParser.addBuffer(data)
sendInitialSql: ->
- payload = new SqlBatchPayload('set textsize ' + @config.options.textsize, @currentTransactionDescriptor())
+ initialSql = 'set textsize ' + @config.options.textsize + '''
+set quoted_identifier on
+set arithabort off
+set numeric_roundabort off
+set ansi_warnings on
+set ansi_padding on
+set ansi_nulls on
+set concat_null_yields_null on
+set cursor_close_on_commit off
+set implicit_transactions off
+set language us_english
+set dateformat mdy
+set datefirst 7
+set transaction isolation level read committed'''
+
+ payload = new SqlBatchPayload(initialSql, @currentTransactionDescriptor())
@messageIo.sendMessage(TYPE.SQL_BATCH, payload.data)
processedInitialSql: ->
Something went wrong with that request. Please try again.