Hi Jeremy, Thanks, I'd like to work on something like this for you when I get back from Nashville. We previously discussed a similar strategy for doing something similar for the default connection options that more closely mimics SSMS, and the AR adapter rather than the raw DB Lib options. I agree with your notes about TEXT vs NVARCHAR(max) -- Most of the sql functions require TEXT columns to get explicitly cast to nvarchar, and it's rather annoying! The trend is definitely to use NVARCHAR.
I'll get back in touch when I've had a chance to play with these ideas a bit and get guidance on how you feel. Thanks! High five! On Tue, Nov 19, 2019 at 11:09 PM Jeremy Evans <jeremyeva...@gmail.com> wrote: > 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 a topic in the > Google Groups "sequel-talk" group. > To unsubscribe from this topic, visit > https://groups.google.com/d/topic/sequel-talk/1O3LPGmAirg/unsubscribe. > To unsubscribe from this group and all its topics, send an email to > sequel-talk+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/sequel-talk/dd32ec33-956c-4b95-a871-f0e709dc4000%40googlegroups.com > <https://groups.google.com/d/msgid/sequel-talk/dd32ec33-956c-4b95-a871-f0e709dc4000%40googlegroups.com?utm_medium=email&utm_source=footer> > . > -- Tim Tilberg 218-491-4637 -- 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 sequel-talk+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/sequel-talk/CAH1O2AH1%2Byr-C_O_4zBaokz5bEvhydwCpdmxZUmEpaiaBnihGA%40mail.gmail.com.
