New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create timestamp columns with millisecond precision on MySQL 5.6 and newer #2542

Merged
merged 5 commits into from Apr 18, 2018

Conversation

Projects
None yet
2 participants
@ricardograca
Collaborator

ricardograca commented Mar 24, 2018

This changes the timestamp() and timestamps() schema building methods to output columns that have millisecond precision on MySQL versions 5.6 and newer.

This feature is not enabled by default for now and requires passing a version: '5.6' option when initializing the client to enable. The version number should be your actual MySQL server version, although as long as it is 5.6 or greater the feature will be enabled. The version number can be in the form 5.6 or 5.6.10.

Fixes #2524.

Associated documentation update: knex/documentation#99

@ricardograca

This comment has been minimized.

Collaborator

ricardograca commented Mar 24, 2018

@elhigu This is still missing the deprecation warning. Any tips on where I should place it? It's also missing documentation but I guess that goes in the documentation repo.

@ricardograca

This comment has been minimized.

Collaborator

ricardograca commented Mar 24, 2018

No idea why the tests are failing on Node 6 and 8. On Node 6 it seems there's a segmentation fault right after the mocha tests. On 8 an assertion related to migrations is failing. I don't think this is caused by the changes in this PR.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Mar 24, 2018

8 failed to some random fail of oracle tests that fails every now and then. That segmentation fault was strange... I just reran those failing builds.

Yes, docs go to other repo and deprecation message inside that supportsPreciseTimestamps function if client.version is not given. Warning should explain that on older mysql versions they need to pass version: '5.5' or something like that or in future that code will break and with newer ones they need to choose 5.5 or older to keep timestamp precision bad. Maybe linking to documentation would be easieast to prevent having that huge warning message.

Increase precision of timestamps to microsecond
- Six is the maximum that it supports and there's no reason not to use
the maximum available precision.
@ricardograca

This comment has been minimized.

Collaborator

ricardograca commented Mar 25, 2018

@elhigu Updated. I also increased the precision to microsecond since this should help reduce rounding errors and I see no reason not to use the highest available precision.

@@ -7,6 +7,17 @@ import * as helpers from '../../../helpers';
import { assign } from 'lodash'
function supportsPreciseTimestamps(client) {
if (!client.version) {

This comment has been minimized.

@elhigu

elhigu Apr 2, 2018

Collaborator

I just realised that actually every mysql user needs to set that variable to get rid of the warning 😬 So I think the way to go there is to change a error message a bit to be like:

'To get rid of this warning one should specify mysql dialect flavor in the knex configuration. Currently flavor defaults to 5.5, but in future releases it will default to 5.7 that supports high precision timestamps. See http://knexjs.org/#Schema-timestamps for more information."

And then just remove the warning in some future release and change the default.

This comment has been minimized.

@ricardograca

ricardograca Apr 2, 2018

Collaborator

Ok, will do.

This comment has been minimized.

@ricardograca

ricardograca Apr 12, 2018

Collaborator

Updated.

@elhigu

elhigu approved these changes Apr 18, 2018

@elhigu elhigu merged commit 14ac2cb into tgriesser:master Apr 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 18, 2018

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment