Make t.timestamps with precision by default.#34970
Conversation
fd2746b to
5fcf8e9
Compare
There was a problem hiding this comment.
Would this not cause warnings of unused arguments?
There was a problem hiding this comment.
Currently Ruby doesn't raise any warnings of unused method arguments for now.
There was a problem hiding this comment.
Just keep db/schema.rb as before.
There was a problem hiding this comment.
Why do we need these similar overrides?
There was a problem hiding this comment.
It is to inject V5_2::TableDefinition to the table definition instance.
https://github.com/rails/rails/pull/34970/files#diff-2a8be25f82da6b3935cc6a41300a1b01R19`
If the change_table is removed, the test_timestamps_doesnt_set_precision_on_change_table is failed.
https://github.com/rails/rails/pull/34970/files#diff-ec048630132cce280a9445022318906eR140
There was a problem hiding this comment.
Should this be supports_datetime_with_precision??
There was a problem hiding this comment.
supports_datetime_with_precision? can't be used in the ActiveRecord::TestCase scope.
subsecond_precision_supported? is defined in the top level as a helper.
5fcf8e9 to
ec47e62
Compare
ec47e62 to
57015cd
Compare
| t.datetime "created_at", null: false | ||
| t.datetime "updated_at", null: false | ||
| t.datetime "created_at", precision: 6, null: false | ||
| t.datetime "updated_at", precision: 6, null: false |
There was a problem hiding this comment.
I've changed to apply new default the same with Action Mailbox.
Currently, if `:datetime` type has a precision, type casting always does round off subseconds regardless of whether necessary or not, it is a bit slow. Since rails#34970, `t.timestamps` with `precision: 6` by default, so `pluck(:created_at)` in newly created app will always be affected by the round off. This avoids the round off if possible, it makes `pluck(:created_at)` about 25% faster. https://gist.github.com/kamipo/e029539f2632aee9f5e711fe66fc8842 Before (0a87d7c with postgresql adapter): ``` Warming up -------------------------------------- users.pluck(:id) 344.000 i/100ms users.pluck(:name) 336.000 i/100ms users.pluck(:created_at) 206.000 i/100ms Calculating ------------------------------------- users.pluck(:id) 3.620k (± 8.5%) i/s - 18.232k in 5.077316s users.pluck(:name) 3.579k (± 9.4%) i/s - 17.808k in 5.020230s users.pluck(:created_at) 2.069k (± 8.0%) i/s - 10.300k in 5.019284s ``` Before (0a87d7c with mysql2 adapter): ``` Warming up -------------------------------------- users.pluck(:id) 245.000 i/100ms users.pluck(:name) 240.000 i/100ms users.pluck(:created_at) 180.000 i/100ms Calculating ------------------------------------- users.pluck(:id) 2.548k (± 9.4%) i/s - 12.740k in 5.066574s users.pluck(:name) 2.513k (± 8.0%) i/s - 12.480k in 5.011260s users.pluck(:created_at) 1.771k (±11.2%) i/s - 8.820k in 5.084473s ``` After (this change with postgresql adapter): ``` Warming up -------------------------------------- users.pluck(:id) 348.000 i/100ms users.pluck(:name) 357.000 i/100ms users.pluck(:created_at) 254.000 i/100ms Calculating ------------------------------------- users.pluck(:id) 3.628k (± 8.2%) i/s - 18.096k in 5.024748s users.pluck(:name) 3.624k (±12.4%) i/s - 17.850k in 5.020959s users.pluck(:created_at) 2.567k (± 7.0%) i/s - 12.954k in 5.081153s ``` After (this change with mysql2 adapter): ``` Warming up -------------------------------------- users.pluck(:id) 268.000 i/100ms users.pluck(:name) 265.000 i/100ms users.pluck(:created_at) 207.000 i/100ms Calculating ------------------------------------- users.pluck(:id) 2.586k (±10.9%) i/s - 12.864k in 5.050546s users.pluck(:name) 2.543k (±10.2%) i/s - 12.720k in 5.067726s users.pluck(:created_at) 2.263k (±14.5%) i/s - 10.971k in 5.004039s ```
Make `t.timestamps` with precision by default rails/rails#34970
Make `t.timestamps` with precision by default rails/rails#34970
|
@kamipo Do you think there's any benefit to documenting that this is incompatible w/ MySQL 5.5? I ran across that issue the other day (the shared ClearDB environments for Heroku don't allow higher versions than 5.5) and it took me a while to figure out what was happening. If so, I'm happy to contribute it. |
|
What is the incompatible you mean? |
|
|
|
@kamipo Do you think there's any benefit to documenting that this is incompatible w/ MySQL 5.5? I ran across that issue the other day (the shared ClearDB environments for Heroku don't allow higher versions than 5.5) and it took me a while to figure out what was happening. If so, I'm happy to contribute it. |
|
What is the incompatible you mean? |
|
|
|
@kamipo I'm curious what the reasoning was for this change. Is there something that makes lessening the default precision on databases that support higher precision (e.g., PostgreSQL) for timestamps desirable? |
… range Now I'm developing a feature like monthly reports. The result of this query doesn't contain the data of the last one sec of this month (from `2021-09-30 23:59:59` to `2021-10-01 00:00:00`). ``` b = Time.current.beginning_of_month # => Wed, 01 Sep 2021 00:00:00.000000000 JST +09:00 e = Time.current.end_of_month # => Thu, 30 Sep 2021 23:59:59.999999999 JST +09:00 User.where(created_at: b..e).to_sql #=> "SELECT `users`.* FROM `users` WHERE `users`.`created_at` BETWEEN '2021-09-01 00:00:00' AND '2021-09-30 23:59:59'" ``` I think `to_s(:db)` should support microsecond, bacuase currently [the default precision of timestamps is six](rails#34970).
|
@kamipo I'm still interested in what the reasoning for this was. Following the links to the discussion it appears this came out of a discussion about MySQL, but this change affected not just MySQL but also databases like Postgres that don't need this change. And I can't find any discussion at all explaining why the change is desirable or was made in the first place. |
|
In fact the comment actually says this should be for MySQL only and be omitted on PostgreSQL, but that's not what this PR ending up doing. |
|
@jcoleman Unfortunately, you might not be able to get a response on a 3 or 4 year old PR. If there is a bug that you've identified, I recommend opening a new issue, or submitting a PR which applies your desired behavior. 🙏 |
The `timestamps` tests for rails#34970 and a939506 are mostly overlapped, so consolidate these tests as "test timestamps with implicit defalut".
I'm not sure it is what we want make it in Rails 6.0.
But I've just implemented to ease to discuss that on this PR.
Context: #34956 (comment)