Set precision 6 by default for datetime columns #42297

robertomiranda · 2021-05-26T15:14:39Z

Summary

Set precision 6 by default for datetime columns, this is the standard behaviour for timestamps #34970 it would good to extend this to all datetimes by default. I've seen this really useful in general when implementing functions relying on time comparison, like cursor pagination. Also, this helps when delivering messages to external services and the ordering matters, or when monotonicity is required.

I'm looking for feedback on this PR and eventually, I will add support to the rest of the adapters supporting precision for datetime and the proper updates around tests and docs. So far I've been trying to find the downside of having a microsecond based timestamp over a second based one by default, the only downside that I've found is the fact that it will require more space, however, the disk space these days are quite cheap wdyt?

zzak · 2021-05-26T22:40:29Z

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb

+
+        NATIVE_DATABASE_TYPES.dup.tap do |types|
+          type[:datetime][:precision] = 6
+        end


This is just a code knit and not at all related to the feature you're trying to work on 😅 , but how about to write this as:

if supports_datetime_with_precision? return NATIVE_DATABASE_TYPES.dup.blahblah end return NATIVE_DATABASE_TYPES

The returns could be implicit I guess, but I try to avoid unless here if you're planning to add more types.

I think it would be better to memoize this, same as we do on https://github.com/rails/rails/blob/main/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L345

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb

zzak · 2021-05-27T13:31:41Z

@robertomiranda I'm also seeing a few failures in CI such as:


Failure:
--
&nbsp; | ActiveRecord::Migration::ChangeSchemaTest#test_change_column_with_timestamp_type [/rails/activerecord/test/cases/migration/change_schema_test.rb:338]:
&nbsp; | --- expected
&nbsp; | +++ actual
&nbsp; | @@ -1 +1 @@
&nbsp; | -"timestamp without time zone"
&nbsp; | +"timestamp(6) without time zone"

and a few others mentioned here, could you check them? 🙇

robertomiranda · 2021-05-27T15:49:39Z

and a few others mentioned here, could you check them?

@zzak sure, I'm on it 👍 🙇

zzak · 2021-05-27T23:11:16Z

@robertomiranda Seeing these now, I think they are related:

/usr/local/bundle/bundler/gems/mysql2-ca883e1a359a/lib/mysql2/client.rb:131:in `_query': Invalid default value for 'modified_datetime' (Mysql2::Error)

ghiculescu · 2021-06-02T17:25:12Z

@robertomiranda thanks for working on this. I agree with the change, but it needs to go through the migration versioning system so that it is backward compatible with past Rails versions.

If you look at #41084 I added a sort of similar feature so you can probably copy the pattern there. Specifically c4290da and the discussion from #41084 (comment) might be helpful. Happy to help further.

robertomiranda · 2021-06-02T20:05:45Z

@ghiculescu thank you for the reference, I will have a look 👍

robertomiranda · 2021-06-02T23:22:33Z

Those changes are looking good to me. Can you add a changelog entry?

Done in b716f916b9, I do still have pending to fix/update a couple of test cases for Postgres and MySQL though 😅

zzak · 2021-06-03T02:46:17Z

@robertomiranda No rush, let us know if you need a review or have any questions! 🙇

robertomiranda · 2021-06-05T11:45:00Z

@zzak @ghiculescu test fixed! ✅

There is one tiny thing around a test case introduced in #41084 which test custom date time on Postgres

 PostgresqlTimestampWithZone.connection.execute("CREATE TYPE custom_time_format AS ENUM ('past', 'present', 'future');")

Basically, the custom type does not support precision you can see the error on this PR #42401. I ended up setting the precision as nil 4762477 .

wdyt?

zzak · 2021-06-07T07:14:04Z

activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb

@@ -411,6 +411,8 @@ def column(name, type, index: nil, **options)
          end
        end

+        options[:precision] ||= 6 if type == :datetime && !options.key?(:precision) && @conn.supports_datetime_with_precision?


Could you make this block form? More than 2 conditions and this line gets very long to read

done in 73c30a4e2a3a44ab1467ef7599bbabbb089eb5b9

zzak · 2021-06-07T11:42:11Z

@robertomiranda Thanks! Can you rebase and squash please? 🙇

ghiculescu · 2021-06-07T14:15:34Z

@zzak @ghiculescu test fixed! ✅

There is one tiny thing around a test case introduced in #41084 which test custom date time on Postgres
 PostgresqlTimestampWithZone.connection.execute("CREATE TYPE custom_time_format AS ENUM ('past', 'present', 'future');")
Basically, the custom type does not support precision you can see the error on this PR #42401. I ended up setting the precision as nil 4762477 .

wdyt?

I think that approach is fine 👍

robertomiranda · 2021-06-15T08:29:16Z

@zzak @ghiculescu 👋 is there anything pending to cover on this PR?

ghiculescu · 2021-06-15T14:04:15Z

@robertomiranda it looks good to me, other than the merge conflict. As it's a new feature it will require someone from the core team to merge it. They are reasonably active but also busy people, and there's lots of PRs open! But sit tight, they'll get to it :)

robertomiranda · 2021-06-15T17:05:13Z

@robertomiranda it looks good to me, other than the merge conflict. As it's a new feature it will require someone from the core team to merge it. They are reasonably active but also busy people, and there's lots of PRs open! But sit tight, they'll get to it :)

Awesome, thanks @ghiculescu, just wanted the know if there was something missing 👌

guilleiguaran

Please rebase

robertomiranda · 2021-06-25T16:10:46Z

Thank you all for help on this PR 🙇

ghiculescu · 2021-12-20T20:00:58Z

#43909 and #43934 seem related to this. @robertomiranda are you able to take a look?

By rails#42297, the default value of datetime precision in rails is now 6. With this change, if the value of datetime precision is 0, it should be dumped into schema, but it is not. So I modified it to dump to schema when the value of datetime precision is 0, and not to dump when the value is 6, which is the default value of rails.

Since #42297, Rails now generates datetime columns on MySQL with a default precision of 6. This means that users upgrading to Rails 7.0 from 6.1, when loading the database schema, would get the new precision value, which would not match the production schema. To avoid problems like this in the future, Rails will now freeze `ActiveRecord::Schema` class to be the 7.0 implementation, and will allow access to other version using the same mechanism of `ActiveRecord::Migration`. The schema dumper will generate the new format which will include the Rails version and will look like this: ``` ActiveRecord::Schema[7.0].define ``` Related to #43934 and #43909.

By rails#42297, the default value of datetime precision in rails is now 6. With this change, if the value of datetime precision is 0, it should be dumped into schema, but it is not. So I modified it to dump to schema when the value of datetime precision is 0, and not to dump when the value is 6, which is the default value of rails.

Since rails#42297, Rails now generates datetime columns on MySQL with a default precision of 6. This means that users upgrading to Rails 7.0 from 6.1, when loading the database schema, would get the new precision value, which would not match the production schema. To avoid problems like this in the future, Rails will now freeze `ActiveRecord::Schema` class to be the 7.0 implementation, and will allow access to other version using the same mechanism of `ActiveRecord::Migration`. The schema dumper will generate the new format which will include the Rails version and will look like this: ``` ActiveRecord::Schema[7.0].define ``` Related to rails#43934 and rails#43909.

rails-bot bot added the activerecord label May 26, 2021

zzak reviewed May 26, 2021

View reviewed changes

zzak requested a review from kamipo May 26, 2021 22:41

zzak reviewed May 27, 2021

View reviewed changes

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb Outdated Show resolved Hide resolved

robertomiranda force-pushed the r/default-precision-datime branch from 57c84ba to e8349e8 Compare June 2, 2021 22:09

This comment has been minimized.

Sign in to view

robertomiranda mentioned this pull request Jun 5, 2021

Add precision test case for custom time type #42401

Closed

robertomiranda force-pushed the r/default-precision-datime branch 2 times, most recently from ac0734a to d5caad5 Compare June 5, 2021 09:23

zzak reviewed Jun 7, 2021

View reviewed changes

robertomiranda force-pushed the r/default-precision-datime branch from 73c30a4 to 789a7a7 Compare June 7, 2021 12:23

zzak approved these changes Jun 9, 2021

View reviewed changes

robertomiranda force-pushed the r/default-precision-datime branch from 789a7a7 to c25f6ef Compare June 9, 2021 08:48

robertomiranda force-pushed the r/default-precision-datime branch from c25f6ef to f2cedf4 Compare June 15, 2021 08:31

ghiculescu added the ready PRs ready to merge label Jun 15, 2021

guilleiguaran approved these changes Jun 25, 2021

View reviewed changes

guilleiguaran self-assigned this Jun 25, 2021

robertomiranda force-pushed the r/default-precision-datime branch from f2cedf4 to c2a6f61 Compare June 25, 2021 08:23

guilleiguaran merged commit 5a26648 into rails:main Jun 25, 2021

robertomiranda deleted the r/default-precision-datime branch June 25, 2021 16:08

This was referenced Jun 25, 2021

Fix migration compatibility for default precision value on datetime columns #42606

Merged

Fix migration compatibility for default precision value on datetime columns (Round 2) #42631

Merged

hotatekaoru mentioned this pull request Sep 23, 2021

Mysql2::Error occured when execute migration datetime column with default option in Rails 7.0 #43292

Closed

mpg-tomohiko-mimura mentioned this pull request Dec 17, 2021

DB schema dumper uncompatibility in Rails 7.0 #43909

Closed

y0t4 mentioned this pull request Jan 14, 2022

Fix MySQL::SchemaDumper behavior about datetime precision value #44171

Merged

rafaelfranca mentioned this pull request Jan 28, 2022

Dump the database schema containing the current Rails version #44286

Merged

kamipo mentioned this pull request Feb 8, 2022

Set the ActiveRecord::Schema version when running the app:update task #44356

Merged

sankichi92 mentioned this pull request Feb 12, 2022

Ridgepole always changes datetime columns after updating ActiveRecord to v7.0 ridgepole/ridgepole#381

Closed

robertomiranda mentioned this pull request Mar 2, 2022

Revert "Set microsencods precision as default value for datetime columns" cookpad/cookpad_mysql_defaults#3

Closed

thoughtbot-summer mentioned this pull request Aug 5, 2022

Keep migrations consistent after Rails upgrades rkrage/pg_party#68

Merged

sambostock mentioned this pull request Sep 22, 2022

Fix erroneous nil default precision on virtual datetime columns #46110

Merged

9 tasks

razumau mentioned this pull request May 17, 2023

add tests on ruby 3.1, 3.2, rails 7.0, drop support for rails 5.1 zendesk/resque-durable#22

Merged

willnet mentioned this pull request Aug 17, 2023

Upgrading to Rails 7.0 changes precision when changing column type to datetime #48965

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set precision 6 by default for datetime columns #42297

Set precision 6 by default for datetime columns #42297

robertomiranda commented May 26, 2021 •

edited

zzak May 26, 2021

ghiculescu Jun 2, 2021

zzak commented May 27, 2021

robertomiranda commented May 27, 2021

zzak commented May 27, 2021

ghiculescu commented Jun 2, 2021

robertomiranda commented Jun 2, 2021

This comment has been minimized.

robertomiranda commented Jun 2, 2021

zzak commented Jun 3, 2021

robertomiranda commented Jun 5, 2021

zzak Jun 7, 2021

robertomiranda Jun 7, 2021

zzak commented Jun 7, 2021

ghiculescu commented Jun 7, 2021

robertomiranda commented Jun 15, 2021

ghiculescu commented Jun 15, 2021

robertomiranda commented Jun 15, 2021 •

edited

guilleiguaran left a comment

robertomiranda commented Jun 25, 2021

ghiculescu commented Dec 20, 2021

Set precision 6 by default for datetime columns #42297

Set precision 6 by default for datetime columns #42297

Conversation

robertomiranda commented May 26, 2021 • edited

Summary

zzak May 26, 2021

Choose a reason for hiding this comment

ghiculescu Jun 2, 2021

Choose a reason for hiding this comment

zzak commented May 27, 2021

robertomiranda commented May 27, 2021

zzak commented May 27, 2021

ghiculescu commented Jun 2, 2021

robertomiranda commented Jun 2, 2021

This comment has been minimized.

robertomiranda commented Jun 2, 2021

zzak commented Jun 3, 2021

robertomiranda commented Jun 5, 2021

zzak Jun 7, 2021

Choose a reason for hiding this comment

robertomiranda Jun 7, 2021

Choose a reason for hiding this comment

zzak commented Jun 7, 2021

ghiculescu commented Jun 7, 2021

robertomiranda commented Jun 15, 2021

ghiculescu commented Jun 15, 2021

robertomiranda commented Jun 15, 2021 • edited

guilleiguaran left a comment

Choose a reason for hiding this comment

robertomiranda commented Jun 25, 2021

ghiculescu commented Dec 20, 2021

robertomiranda commented May 26, 2021 •

edited

robertomiranda commented Jun 15, 2021 •

edited