On Tuesday, November 19, 2019 at 7:47:24 PM UTC-8, Tim Tilberg wrote: > > Tonight I noticed a small detail that surprised me a little bit, and I > wanted to discuss whether it should be changed: > > When doing migrations against SQL Server using the TinyTDS adapter, the > default `String` column datatype is `varchar` instead of `nvarchar`. > > There are several reasons this surprised me: > > - From my own DB experience with SQL Server, the default string column > type is usually advocated as NVARCHAR for general compatibility > - Looking at other parts of the Sequel codebase, it seems that `nvarchar` > is often used as default assumptions in other custom parts for SQL Server: > - Prepared statement datatypes: > > https://github.com/jeremyevans/sequel/blob/9202d780b92626646c9faeff90a7f7b9d7b6c10d/lib/sequel/adapters/tinytds.rb#L185 > - The output of sprocs (Honestly, I'm not actually sure what I'm looking > at here. This might be irrelevant) > > https://github.com/jeremyevans/sequel/blob/master/lib/sequel/adapters/shared/mssql.rb#L84 > - And importantly*, the default usage of unicode strings (N'CONTENT') > when filtering via `#mssql_unicode_strings` > > https://github.com/jeremyevans/sequel/blob/943350e613123387d75a26114fd9fcced4fea7f5/lib/sequel/adapters/shared/mssql.rb#L21 > - And finally, while this isn't necessarily supposed to be in-hand with > the rails SqlServerAdapter, their default interpretation of String data is > to use NVARCHAR as well > > *As is alluded to in the comments for mssql_unicode_strings, this may be > causing unexpected performance issues. At one point a few months back while > interacting with an existing legacy database, I found that I had to disable > `mssql_unicode_strings` to stop sending N'' strings, yielding much improved > performance against a table that had `VARCHAR` columns instead of NVARCHAR. > I assume the opposite is potentially true as well, and if it is then the > default experience puts people into a performance issue from the beginning. > > Jeremy, would you be willing to accept this as an issue, and accept a PR > for it? (Also, side note, are you at Rubyconf? I'd love to give you a high > five.) >
Unfortunately, I don't think the default behavior can be changed due to backwards compatibility, as it would change the behavior of all existing migrations. However, I'll definitely accept an mssql shared adapter option to make the default string column type be nvarchar. It's possible we could change the default in Sequel 6, as long as we retain the option and allow for the current behavior. We should also consider how to handle the :text and :fixed String type options. As ntext appears to be going out of style, nvarchar(max) for :text and nchar for :fixed seem like the best choices, but I'm not a Microsoft SQL Server expert. We might have to have use ntext if nvarchar(max) is not supported in the version of the database being accessed. Unfortunately, I wasn't able to make it to RubyConf this year. Virtual high fives are accepted :) Thanks, Jeremy -- You received this message because you are subscribed to the Google Groups "sequel-talk" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/sequel-talk/dd32ec33-956c-4b95-a871-f0e709dc4000%40googlegroups.com.
