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

Active Record + PostgreSQL: native support for timestamp with time zone #41084

Merged
merged 1 commit into from May 8, 2021

Conversation

ghiculescu
Copy link
Member

@ghiculescu ghiculescu commented Jan 12, 2021

In #21126 it was suggested to make "timestamp with time zone" the default type for datetime columns in PostgreSQL. This is in line with PostgreSQL best practices. This PR lays some groundwork for that.

This PR adds a configuration option, ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.datetime_type. The default is :timestamp which preserves current Rails behavior of using "timestamp without time zone" when you do t.datetime in a migration. If you change it to :timestamptz, you'll get "timestamp with time zone" columns instead.

If you change this setting in an existing app, you should immediately call bin/rails db:migrate to ensure your schema.rb file remains correct. If you do so, then existing columns will not be impacted, so for example if you have an app with a mixture of both types of columns, and you change the config, schema dumps will continue to output the correct types.

This PR also adds two new types that can be used in migrations: t.timestamp and t.timestamptz.

ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.datetime_type = :timestamp # default value is :timestamp

create_table("foo1") do |t|
  t.datetime :default_format # "timestamp without time zone"
  t.timestamp :without_time_zone # "timestamp without time zone"
  t.timestamptz :with_time_zone # "timestamp with time zone"
end

ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.datetime_type = :timestamptz

create_table("foo2") do |t|
  t.datetime :default_format # "timestamp with time zone" <-- note how this has changed!
  t.timestamp :without_time_zone # "timestamp without time zone"
  t.timestamptz :with_time_zone # "timestamp with time zone"
end

ActiveRecord::ConnectionAdapters::PostgreSQLAdapter::NATIVE_DATABASE_TYPES[:my_custom_type] = { name: "custom_datetime_format_i_invented" }
ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.datetime_type = :my_custom_type

create_table("foo3") do |t|
  t.datetime :default_format # "custom_datetime_format_i_invented"
  t.timestamp :without_time_zone # "timestamp without time zone"
  t.timestamptz :with_time_zone # "timestamp with time zone"
end

Notes

@ghiculescu ghiculescu force-pushed the default-pg-datetime-format branch 2 times, most recently from ccf651b to 4d0a141 Compare January 12, 2021 03:21
@ghiculescu ghiculescu changed the title Change default PostgreSQL datetime format to timestamp with time zone Change default PostgreSQL datetime format to timestamp with time zone Jan 12, 2021
@ghiculescu ghiculescu force-pushed the default-pg-datetime-format branch 10 times, most recently from d223fe9 to a5880ec Compare January 13, 2021 21:21
@ghiculescu ghiculescu force-pushed the default-pg-datetime-format branch 3 times, most recently from 48295ec to b25f51c Compare January 13, 2021 22:59
@ghiculescu ghiculescu marked this pull request as ready for review January 13, 2021 23:16
Base automatically changed from master to main January 14, 2021 17:03
@ghiculescu ghiculescu force-pushed the default-pg-datetime-format branch 3 times, most recently from c3aba26 to 8ac0517 Compare February 9, 2021 19:12
ghiculescu added a commit to ghiculescu/buildkite-config that referenced this pull request Feb 9, 2021
Tests Active Record using Postgres with `timestamp` (not `timestamptz`) as the default datetime column type. rails/rails#41084 adds the relevant task, so that needs to be merged first.
@ghiculescu ghiculescu force-pushed the default-pg-datetime-format branch 2 times, most recently from e0e3421 to d576a59 Compare February 9, 2021 23:17
@ghiculescu
Copy link
Member Author

@rafaelfranca tagging you in since you commented here #21126 (comment) (but let me know if I should tag someone else - you get tagged in for everything). How's this looking so far?

I'm not confident about the CI bit rails/buildkite-config#11 and don't really know how to test that it will work. I have been running POSTGRESQL_DATETIME_TYPE=timestamp bundle exec rake test:postgresql locally to confirm tests pass. The way that's implemented is fairly bad too, but I couldn't find/think of any better approaches.

@Dearest
Copy link

Dearest commented Feb 23, 2022

@ghiculescu
I'll elaborate on the problem I'm having and I hope you can help me.

I am trying to upgrade Rails 6.x to Rails 7.x.

I have a project that connects to multiple databases. In these databases some time fields are of type timestamp and some fields are timestamptz. This means that I have both timestamp and timestamptz in the databases I am connected to.
Now I have a model called Post, which has a published_at attribute, and this field is of type timestamptz in the database (this column is use sql create, not rails migration.)

# Rails6.1.4.2
Post.last.published_at.class
# ==> ActiveSupport:: TimeWithZone
Post.last.published_at.to_s
# ==> 2022-02-23 10:33:36

everything is fine
When I upgraded to Rails 7. same model, same fields

# Rails 7.0.1
Post.last.published_at.class
# ==> Time
Post.last.published_at.to_s
# ==> 2022-02-23 02:33:36 UTC

To summarize: Rails 6.x formats timestamp as ActiveSupport::TimeWithZone, but Rails 7.x formats the same field as Time. But I didn't see this discussed in the issue, so I don't know what went wrong

@ghiculescu
Copy link
Member Author

ghiculescu commented Mar 2, 2022

@Dearest that executable test needs ActiveRecord::Base.time_zone_aware_attributes = true, then it will pass on Rails 6 or 7. (That is needed because time_zone_aware_attributes is set in a Railtie, which isn't run in the executable test case by default. But it is run when you start the app/console.)

I have a feeling I know what the issue is. Could you please get me the values of

ActiveRecord::Base.time_zone_aware_attributes
ActiveRecord::Base.time_zone_aware_types
ActiveRecord::Base.skip_time_zone_conversion_for_attributes

In both your Rails 6 and 7 versions. I have a feeling time_zone_aware_types will equal [:datetime, :time]. If it does, see if adding :timestamptz to that array fixes the issue, by doing this in an initializer

ActiveRecord::Base.time_zone_aware_types += [:timestamptz]

@Dearest
Copy link

Dearest commented Mar 2, 2022

@ghiculescu COOL! , I use this fix the issue

ActiveRecord::Base.time_zone_aware_types += [:timestamptz]

Before add ActiveRecord::Base.time_zone_aware_types += [:timestamptz]

Loading development environment (Rails 7.0.1)
[1] pry(main)> ActiveRecord::Base.time_zone_aware_attributes
=> true
[2] pry(main)> ActiveRecord::Base.time_zone_aware_types
=> [:datetime, :time]
[3] pry(main)> ActiveRecord::Base.skip_time_zone_conversion_for_attributes
=> []
Loading development environment (Rails 6.1.4.1)
[1] pry(main)> ActiveRecord::Base.time_zone_aware_attributes
=> true
[2] pry(main)> ActiveRecord::Base.time_zone_aware_types
=> [:datetime, :time]
[3] pry(main)> ActiveRecord::Base.skip_time_zone_conversion_for_attributes
=> []

@Dearest
Copy link

Dearest commented Mar 2, 2022

@ghiculescu But I got another issue
in Rails6. User.last.created_at.class is ActiveSupport::TimeWithZone. And I use to_s,I will get result like this

Loading development environment (Rails 6.1.4.1)
[1] pry(main)> User.last.created_at.to_s
=> "2021-12-08 18:13:34"

But Rails7. I will get string include time zone

Loading development environment (Rails 7.0.1)
[1] pry(main)> User.last.created_at.to_s
=> "2021-12-08 18:13:34 +0800"

Can I configure the behavior of to_s . I don't want to use the monkey patch to hack to_s method

@ghiculescu
Copy link
Member Author

ghiculescu commented Mar 2, 2022

@Dearest is User.last.created_at.class the same in both Rails versions? What is the database type of User#created_at? And what is ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.datetime_type?

btw I see you're on 7.0.1. If you are upgrading now, you should try going straight to 7.0.2.2.

ghiculescu added a commit to ghiculescu/rails that referenced this pull request Jul 5, 2022
rails#41395 added support for the `timestamptz` type on the Postgres adapter.

As we found [here](rails#41084 (comment)) this causes issues because in some scenarios the new type is not considered a time zone aware attribute, meaning values of this type in the DB are presented as a `Time`, not an `ActiveSupport::TimeWithZone`.

This PR fixes that by ensuring that `timestamptz` is always a time zone aware type, for Postgres users.
ghiculescu added a commit to ghiculescu/rails that referenced this pull request Jul 10, 2022
rails#41395 added support for the `timestamptz` type on the Postgres adapter.

As we found [here](rails#41084 (comment)) this causes issues because in some scenarios the new type is not considered a time zone aware attribute, meaning values of this type in the DB are presented as a `Time`, not an `ActiveSupport::TimeWithZone`.

This PR fixes that by ensuring that `timestamptz` is always a time zone aware type, for Postgres users.
oliverguenther added a commit to opf/openproject that referenced this pull request Aug 31, 2022
rails/rails#41084 introduces support for timestamp with time zone

Currently, all our timestamp and datetime fields are without time zone (the previous default of datetime)

This has resulted in some inconsistencies when trying to query for a time object not explicitly passed as UTC
oliverguenther added a commit to opf/openproject that referenced this pull request Aug 31, 2022
rails/rails#41084 introduces support for timestamp with time zone

Currently, all our timestamp and datetime fields are without time zone (the previous default of datetime)

This has resulted in some inconsistencies when trying to query for a time object not explicitly passed as UTC
oliverguenther added a commit to opf/openproject that referenced this pull request Sep 1, 2022
rails/rails#41084 introduces support for timestamp with time zone

Currently, all our timestamp and datetime fields are without time zone (the previous default of datetime)

This has resulted in some inconsistencies when trying to query for a time object not explicitly passed as UTC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants