Datetime column in MSSQL migrations

[kblomster]

In knex.schema, creating or altering a column using table.dateTime() or table.timestamp() generates a query that uses the datetime type. Microsoft discourages the use of this type for new work and recommends using datetime2 instead, since it's got considerably better precision, a wider representable range and is ANSI SQL compliant, without using any more storage (both types are 8 bytes wide by default). datetime2 is available since SQL Server 2008, but since I believe other parts of knex generates syntax requiring at least SQL Server 2012, that shouldn't be a problem. The dominant MSSQL Node driver (Tedious) supports both and returns both as a Javascript Date by default.

Now, this is trivial to work around with table.specificType, but I think knex should default to the recommended best practice. I am willing to create a PR for this, but before I do that I figured I'd check if there are any potential problems I've overlooked with such a change. I don't know what the policy is on backwards compatibility in migrations, either.

In addition to switching to datetime2, MSSQL also has a timezone-aware type, datetimeoffset, which is like datetime2 but including a timezone offset (in Postgres terms it's a timestamptz). Perhaps a better idea would be to implement the same API as Postgres, that is table.dateTime(columnName, withoutTimezone), but unlike just switching from datetime to datetime2 which is technically incompatible but unlikely to actually break things in practice, changing to timezone-aware by default has a great deal of potential for interesting bugs.

[wubzz]

I believe that if Microsoft recommends against the use of datetime then definitely knex should follow this recommendation.

I don't know much about MSSQL let alone the detailed differences between datetime and datetime2, so I can't determine if this would be a breaking change, but nevertheless the upcoming release already contains some breaking changes so I think 0.14 would be good for this.

@elhigu thoughts?

[kblomster]

I don't think it'd be a breaking change in Javascript, really, barring weird edge cases relying on datetime's precision, or something. datetime is just all around worse than datetime2 - most glaring issue is that it has a precision of only 3 milliseconds (i.e. 23:59:59.997), while datetime2 offers 100 nanoseconds (i.e. 23:59:59.9999999) - and the only reason to use it that I'm aware of is for backwards compatibility with old client applications that don't support datetime2. I don't think any such JS clients exist, so that point should be irrelevant.

I think both of them support the same string literal formats for input, as well, although this is only really relevant to queries explicitly using custom and/or localized date formats. I could investigate further if needed.

e: Actually now that I think about it there may be a few more gotchas wrt manipulating datetimes with SQL functions, but I'll have to investigate if this actually is meaningful in practice in this case.

[elhigu]

I wouldnt like to make this as default, but to add some deprecation notice maybe. If we change the type now, it means that when existing project updates knex and runs new migrations they will have mixed datetime types in their db which sound pretty scary...

Im not sure how to handle this.

[kblomster]

@elhigu Well, yes. However, datetime and datetime2 are pretty much interchangeable and can be compared directly without casts. Converting a column from the old to the new format can be done with a simple ALTER TABLE ... ALTER COLUMN (presuming there are no constraints or indexes). I agree that mixed types is a hairy situation to be in but I'm not sure if it actually has any real practical impact in this case. Just using datetime in the first place offers plenty of potential for bugs since the precision is so bad.

There is one thing datetime has that datetime2 doesn't though - it's implicitly convertible to a float (number of days since the epoch), so you can do simple arithmetic and aggregation on it. With datetime2 you must use various SQL functions for that (dateadd & friends).

What would you propose to move this forward though? Add a deprecation warning to table.dateTime() and table.timestamp() for MSSQL and only swap it after the next version bump? Add a new function for datetimeoffset? I don't really like the idea of an inconsistent API though.

[elhigu]

I suppose that one correct flow for this would be allow using datetime2 with .timestamp({ highPrecision: true }) flag or something like that and add deprecation warning that in 0.15 highPrecision: true will be changed to be default. So if they don't add { highPrecision: false } to their timestamp commands, during 0.14, then their code will work differently in 0.15 release.

That option should be implemented also for .timestamps() method and pass it to internal calls to .timestamp().

[kblomster]

That sounds reasonable. I'll take a look at setting up a PR.

[dhensby]

fixed in #2757