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

Add timestamptz as a time zone aware type for PostgreSQL #44601

Merged
merged 1 commit into from Jul 10, 2022

Conversation

ghiculescu
Copy link
Member

@ghiculescu ghiculescu commented Mar 2, 2022

#41395 added support for the timestamptz type on the Postgres adapter.

As we found here 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. A workaround is to do this in an initializer:

ActiveRecord::Base.time_zone_aware_types << :timestamptz

Or if you don't want this you can opt out in an initializer:

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

I think this should be backported to 7-0-stable.

Fixes #45270

@ghiculescu ghiculescu force-pushed the time-zone-aware-type-postgres branch from bcfd233 to b91442d Compare March 2, 2022 19:54
@ghiculescu ghiculescu force-pushed the time-zone-aware-type-postgres branch 2 times, most recently from b1d752a to 3018966 Compare March 4, 2022 17:58
@rails-bot rails-bot bot added the railties label Jun 7, 2022
@ghiculescu ghiculescu force-pushed the time-zone-aware-type-postgres branch 5 times, most recently from c253b14 to 2d2136f Compare June 8, 2022 19:20
@ghiculescu ghiculescu force-pushed the time-zone-aware-type-postgres branch 3 times, most recently from 9416503 to ec322f5 Compare June 30, 2022 23:55
@yahonda
Copy link
Member

yahonda commented Jun 30, 2022

Let me take a look at it.

@yahonda
Copy link
Member

yahonda commented Jul 1, 2022

Before diving into implementation details, Can I get your opinion about adding tstzrange type also as time_zone_aware_types ?

Background is Based #45348

In PostgreSQL, tstzrange is "range of timestamp with time zone" so I think we can define tstzrange as timezone aware types in Rails. on the other hand, tsrange is "Range of timestamp without time zone" so I prefer not to make tsrange type as timezone aware time_zone_aware_types by default. Users can opt-in tsrange as time_zone_aware_types if necessary.

https://www.postgresql.org/docs/current/rangetypes.html

8.17.1. Built-in Range and Multirange Types
PostgreSQL comes with the following built-in range types:

  • tsrange — Range of timestamp without time zone, tsmultirange — corresponding Multirange
  • tstzrange — Range of timestamp with time zone, tstzmultirange — corresponding Multirange

cc @morgoth

@ghiculescu
Copy link
Member Author

Yep, that sounds reasonable. I'll add it to this PR.

@ghiculescu ghiculescu force-pushed the time-zone-aware-type-postgres branch from ec322f5 to 1a22c52 Compare July 1, 2022 02:20
@ghiculescu ghiculescu changed the title Make timestamptz a time zone aware type for Postgres Add timestamptz and tstzrange as time zone aware types for PostgreSQL Jul 1, 2022
@ghiculescu
Copy link
Member Author

@yahonda updated 👍

@morgoth
Copy link
Member

morgoth commented Jul 1, 2022

@yahonda Currently Rails is configured to use timezone awareness for "timestamp" column type.

It's a bit confusing when the AR logic should be used - should it be for columns without "tz" (to add it to columns that do not support it natively) or with "tz" (that supposed to support it natively)?

@ghiculescu
Copy link
Member Author

ghiculescu commented Jul 1, 2022

Hmm, we use this as an example:

# ActiveRecord::Base.time_zone_aware_types += [:tsrange, :tstzrange]

So maybe it's intentionally not included by default. I'll remove it from this PR. We can re-add in a separate PR if needed.

@ghiculescu ghiculescu changed the title Add timestamptz and tstzrange as time zone aware types for PostgreSQL Add timestamptz as a time zone aware type for PostgreSQL Jul 1, 2022
@ghiculescu ghiculescu force-pushed the time-zone-aware-type-postgres branch from 1a22c52 to 3fee3ac Compare July 1, 2022 18:30
@ghiculescu ghiculescu requested a review from yahonda July 1, 2022 18:30
@ghiculescu ghiculescu force-pushed the time-zone-aware-type-postgres branch from 3fee3ac to 236eeff Compare July 1, 2022 19:05
@ghiculescu ghiculescu force-pushed the time-zone-aware-type-postgres branch from 236eeff to eb93a8e Compare July 5, 2022 17:10
@ghiculescu ghiculescu requested a review from yahonda July 5, 2022 17:23
@yahonda
Copy link
Member

yahonda commented Jul 6, 2022

@morgoth Thanks for the explanation. Agreed it is difficult to say timezone aware types in Rails just because the database column is timezone aware. Let's keep it as it is.

@ghiculescu ghiculescu force-pushed the time-zone-aware-type-postgres branch from eb93a8e to 2c931f0 Compare July 6, 2022 14:04
@ghiculescu
Copy link
Member Author

I rebased again. Keeping the changelog up to date is a full time job 😆

@yahonda
Copy link
Member

yahonda commented Jul 9, 2022

Looks good to me. Would you address the conflict with CHANGELOG.md?

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 ghiculescu force-pushed the time-zone-aware-type-postgres branch from 2c931f0 to 75c406d Compare July 10, 2022 20:32
@ghiculescu ghiculescu requested a review from yahonda July 10, 2022 20:34
@yahonda yahonda merged commit 2cf8f37 into rails:main Jul 10, 2022
yahonda added a commit to yahonda/rails that referenced this pull request Jul 10, 2022
…ostgres

Add `timestamptz` as a time zone aware type for PostgreSQL
@yahonda
Copy link
Member

yahonda commented Jul 10, 2022

Considering #45270 this is a regression in Rails 7.0 .z then backported to 7-0-stable via 4cd5709

@ghiculescu ghiculescu deleted the time-zone-aware-type-postgres branch July 11, 2022 00:03
@ghiculescu
Copy link
Member Author

Thanks for all your help @yahonda !

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.

Regression: Setting datetime inside Time#use_zone does not work
3 participants