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.

Sign up for GitHub

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

Jump to bottom

Use correct precision when touching updated_at column in upsert #42993

Merged
merged 4 commits into from
Sep 14, 2021
Merged

Use correct precision when touching updated_at column in upsert #42993

merged 4 commits into from
Sep 14, 2021

Conversation

sambostock
Copy link
Contributor

@sambostock sambostock commented Aug 10, 2021 •
edited

Summary

Closes #42992

InsertAll (the machinery which powers, among others methods, upsert) uses CURRENT_TIMESTAMP with an implicit precision of 0 as the default when touching updated_at/updated_on.

This PR updates it to pass in the current time, serialized according to the column's type.

Not all databases support increased precision for CURRENT_TIMESTAMP (e.g. MySQL supports CURRENT_TIMESTAMP(6), but SQLite does not), so it is simpler to just serialize the current time.

Other Information

⚠️ Before Merging

  • Address TODO comments
  • Add changelog entry
  • Confirm there is no reason CURRENT_TIMESTAMP had been chosen instead of serializing the current time, and that this won't break anything.

@sambostock sambostock force-pushed the use-precision-in-updated-at-upsert branch from 0581979 to 393d244 Compare August 11, 2021 14:20
@sambostock sambostock marked this pull request as ready for review August 11, 2021 14:28
@sambostock sambostock force-pushed the use-precision-in-updated-at-upsert branch from 231adfd to 80a5da1 Compare August 11, 2021 18:25
@sambostock sambostock mentioned this pull request Aug 12, 2021
Merged
4 tasks
@tenderlove
Copy link
Member

I'd prefer we use CURRENT_TIMESTAMP rather than Time.now because we don't have to worry about clock skew, time zone issues, etc between the db and the app server. Looks like SQLite3 could use strftime instead of CURRENT_TIMESTAMP. Can we add a method to each adapter that returns the best SQL for that DB?

@sambostock
Copy link
Contributor Author

sambostock commented Aug 12, 2021 •
edited

Ahh, interesting suggestion, @tenderlove. I hadn't considered offloading that per adapter. Couple questions:

  • Do you know why we take differing approaches in insert_all (CURRENT_TIMESTAMP) vs when we set timestamps on individual records (current_time_from_proper_timezone)?
  • Do you think we should use this approach in Set timestamps on insert_all/upsert_all record creation #43003 (setting created_at, etc. on insert) too?
  • Should we
    • generate a string with the precision matching that specific column?
      e.g. MySQL: updated_at = CURRENT_TIMESTAMP(model.type_for_attribute("updated_at").precision)
    • or always pass in a string for the max precision? (I ended up going with this)
      e.g. MySQL: updated_at = CURRENT_TIMESTAMP(6)
  • Similarly, does it matter how we do this for datetime vs date columns? (It doesn't)
    e.g. SQLite updated_on = STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')

@sambostock
Add high_precision_current_timestamp to adapters
da41061 
CURRENT_TIMESTAMP provides differing precision on different databases.

This method can be used in queries instead to provide a high precision
current time regardless of the database being connected to.

CURRENT_TIMESTAMP continues to be used as the default, unless overriden.
@sambostock
Add precision: 6 to books.{created,updated}_at test table
58cfa92 
Some tests will require precision.
@sambostock
Use full precision for updated_at in insert_all/upsert_all
213c4c5 
`CURRENT_TIMESTAMP` provides differing precision depending on the database,
and not all databases support explicitly specifying additional precision.

Instead, we delegate to the new `connection.high_precision_current_timestamp`
for the SQL to produce a high precision timestamp on the current database.
@sambostock sambostock force-pushed the use-precision-in-updated-at-upsert branch from 0e41cbc to 213c4c5 Compare August 27, 2021 04:30
@sambostock
Copy link
Contributor Author

@tenderlove I've made the changes you requested 👍

sambostock
sambostock commented Aug 27, 2021
View reviewed changes
activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
@@ -441,6 +441,19 @@ def with_yaml_fallback(value) # :nodoc:
end
end

# This is a safe default, even if not high precision on all databases
HIGH_PRECISION_CURRENT_TIMESTAMP = Arel.sql("CURRENT_TIMESTAMP").freeze # :nodoc:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I extracted these so we wouldn't create a new object each time. I saw this done in some other places, which also used :nodoc:.

activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
Comment on lines +453 to +455
def high_precision_current_timestamp
HIGH_PRECISION_CURRENT_TIMESTAMP
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally thought we'd be able to just have this be the only implementation of the method, and simply override the constant in each adapter, but that doesn't seem to work.

We could do self.class::HIGH_PRECISION_CURRENT_TIMESTAMP but that seems wrong.

Another alternative would be to use differently named constants, but I'm not sure that's better.

Copy link
Member

Choose a reason for hiding this comment

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

I originally thought we'd be able to just have this be the only implementation of the method, and simply override the constant in each adapter, but that doesn't seem to work.

We could do self.class::HIGH_PRECISION_CURRENT_TIMESTAMP but that seems wrong.

Meh. I think this is totally fine.

@tenderlove
Copy link
Member

  • Do you know why we take differing approaches in insert_all (CURRENT_TIMESTAMP) vs when we set timestamps on individual records (current_time_from_proper_timezone)?

I don't know the answer to this. Probably using Time.now or whatever just seemed like the easiest at the time (lol).

If it's not too hard, yes.

@tenderlove tenderlove merged commit 0b5c721 into rails:main Sep 14, 2021
sambostock added a commit to sambostock/rails that referenced this pull request Oct 12, 2021
@sambostock
Add high precision creation upsert regression test
134a189 
This test ensures we use the same high precision timestamp for bulk
record creation as we do for bulk record updates.

See rails#42993 for details.
@sambostock sambostock deleted the use-precision-in-updated-at-upsert branch October 13, 2021 18:06
@eugeneius eugeneius mentioned this pull request Dec 28, 2021
Merged
@adrianna-chang-shopify adrianna-chang-shopify mentioned this pull request Mar 3, 2023
Merged
nalabjp added a commit to nalabjp/solid_queue that referenced this pull request May 11, 2024
@nalabjp
Manually add created_at and updated_at to attributes
e1a9400 
In Rails7 the timestamps column is automatically updated when insert_all and upsert_all are executed,
but before that it had to be updated manually.
rails/rails#43003

There was a related pull request that presicion was not taken into account when auto-updating timestamps columns,
but this does not affect `solid_queue` table schema as there is no presicion setting.
rails/rails#42993
nalabjp added a commit to nalabjp/solid_queue that referenced this pull request May 11, 2024
@nalabjp
Manually add created_at and updated_at to attributes
bf35920 
In Rails7 the timestamps column is automatically updated when insert_all and upsert_all are executed,
but before that it had to be updated manually.
rails/rails#43003

There was a related pull request that presicion was not taken into account when auto-updating timestamps columns,
but this does not affect `solid_queue` table schema as there is no presicion setting.
rails/rails#42993
nalabjp added a commit to nalabjp/solid_queue that referenced this pull request May 11, 2024
@nalabjp
Manually add created_at and updated_at to attributes
fd24f81 
In Rails7 the timestamps column is automatically updated when insert_all and upsert_all are executed,
but before that it had to be updated manually.
rails/rails#43003

There was a related pull request that presicion was not taken into account when auto-updating timestamps columns,
but this does not affect `solid_queue` table schema as there is no presicion setting.
rails/rails#42993
nalabjp added a commit to nalabjp/solid_queue that referenced this pull request May 11, 2024
@nalabjp
Manually add created_at and updated_at to attributes
a24a67a 
In Rails7 the timestamps column is automatically updated when insert_all and upsert_all are executed,
but before that it had to be updated manually.
rails/rails#43003

There was a related pull request that presicion was not taken into account when auto-updating timestamps columns,
but this does not affect `solid_queue` table schema as there is no presicion setting.
rails/rails#42993
nalabjp added a commit to nalabjp/solid_queue that referenced this pull request May 12, 2024
@nalabjp
Manually add created_at and updated_at to attributes
b210424 
In Rails7 the timestamps column is automatically updated when insert_all and upsert_all are executed,
but before that it had to be updated manually.
rails/rails#43003

There was a related pull request that presicion was not taken into account when auto-updating timestamps columns,
but this does not affect `solid_queue` table schema as there is no presicion setting.
rails/rails#42993
nalabjp added a commit to nalabjp/solid_queue that referenced this pull request May 12, 2024
@nalabjp
Manually add created_at and updated_at to attributes
c4b9e84 
In Rails7 the timestamps column is automatically updated when insert_all and upsert_all are executed,
but before that it had to be updated manually.
rails/rails#43003

There was a related pull request that presicion was not taken into account when auto-updating timestamps columns,
but this does not affect `solid_queue` table schema as there is no presicion setting.
rails/rails#42993
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tenderlove tenderlove tenderlove left review comments

Assignees
No one assigned
Labels
activerecord
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

upsert does not use correct precision when updating updated_at column.
2 participants
@sambostock @tenderlove