Skip to content
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

ricardograca
Copy link
Member

@ricardograca 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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

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.

- Six is the maximum that it supports and there's no reason not to use
the maximum available precision.
@ricardograca
Copy link
Member Author

@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) {
Copy link
Member

@elhigu elhigu Apr 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@elhigu elhigu merged commit 14ac2cb into knex:master Apr 18, 2018
@elhigu
Copy link
Member

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants