-
Notifications
You must be signed in to change notification settings - Fork 21.9k
Make t.timestamps
with precision by default.
#34970
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
Make t.timestamps
with precision by default.
#34970
Conversation
fd2746b
to
5fcf8e9
Compare
options[:index] ||= false | ||
super | ||
end | ||
alias :add_belongs_to :add_reference | ||
|
||
def add_timestamps(_, **options) | ||
def add_timestamps(table_name, **options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this not cause warnings of unused arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently Ruby doesn't raise any warnings of unused method arguments for now.
@@ -2,7 +2,7 @@ class CreateMessages < ActiveRecord::Migration[6.0] | |||
def change | |||
create_table :messages do |t| | |||
t.string :subject | |||
t.timestamps | |||
t.timestamps precision: nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just keep db/schema.rb
as before.
super { |t| yield compatible_table_definition(t) } | ||
else | ||
super | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need these similar overrides?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -142,4 +142,40 @@ def test_timestamps_without_null_set_null_to_false_on_add_timestamps | |||
assert_not @connection.columns(:has_timestamps).find { |c| c.name == "created_at" }.null | |||
assert_not @connection.columns(:has_timestamps).find { |c| c.name == "updated_at" }.null | |||
end | |||
|
|||
if subsecond_precision_supported? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be supports_datetime_with_precision?
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 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)