Add database helpers 'add_timestamps_with_timezone' and 'timestamps_with_timezone'

What does this MR do?

This MR adds:

1. A database helper add_timestamps_with_timezone based on Rails' add_timestamps method.
2. A database helper timestamps_with_timezone based on Rails' timestamps method.
3. A custom datetime_with_timezone data type:

• for PostgreSQL it converts to timestamptz.
• for MySQL it converts to timestamp.

Why was this MR needed?

We want to store data with timezone information.

Does this MR meet the acceptance criteria?

• Changelog entry added, if necessary
• Tests
  • Added for this feature/bug
  • All builds are passing
• Branch has no merge conflicts with master (if it does - rebase it please)
• Squashed related commits together

What are the relevant issue numbers?

Closes #32054 (closed)

marked the checklist item Changelog entry added, if necessary as completed

marked the checklist item Branch has no merge conflicts with master (if it does - rebase it please) as completed

marked the checklist item Squashed related commits together as completed

added 1 commit

• a62b9426 - Make migrations use 'timestamptz' database type for 'datetime' and 'timestamp'

Compare with previous version

@yorickpeterse, could you review the code, please?

assigned to @yorickpeterse

unmarked as a Work In Progress

added 1 commit

• 086c41bd - Make migrations use 'timestamptz' database type for 'datetime' and 'timestamp'

Compare with previous version

assigned to @blackst0ne

changed title from Resolve "Rails should use timestamptz database type for PostgreSQL" to Add database helper 'add_timestamp_with_timezone'

marked as a Work In Progress

added backstage label

Since this doesn't apply to migrations retroactively, does this create the possibility of differing behavior between a fresh GitLab install created after this is released, and one that's just been upgraded?

Or even within the same installation -- issues.created_at on a legacy install like gitlab.com will be a regular timestamp, but if we add a new table it will use timestamptz. Will that cause issues?

added 455 commits

• 086c41bd...9fde881d - 454 commits from branch master
• c4b7b1d1 - Add database helper 'add_timestamp_with_timezone'

Compare with previous version

changed the description

marked the checklist item Added for this feature/bug as completed

added 1 commit

• 87517bcc - Add database helper 'add_timestamp_with_timezone'

Compare with previous version

@yorickpeterse, database helper has been added.

Could you review the code, please?

assigned to @yorickpeterse

added 157 commits

• 87517bcc...bcc504d3 - 156 commits from branch master
• ca9c7abe - Add database helper 'add_timestamp_with_timezone'

Compare with previous version

added 157 commits

• 87517bcc...bcc504d3 - 156 commits from branch master
• ca9c7abe - Add database helper 'add_timestamp_with_timezone'

Compare with previous version

unmarked as a Work In Progress

changed the description

marked the checklist item All builds are passing as completed

@blackst0ne In your earlier version you simply re-defined :datetime and such to use timestamptz. Is it possible to define a separate type called :timestamptz that on PostgreSQL uses the same type, but :datetime on MySQL? That way we can just use add_column :table, :column, :timestamptz, without having to worry about messing up migrations that are expected to use :datetime. Sorry for bouncing you around

assigned to @blackst0ne

@yorickpeterse, so we have come where we started.

Should we disallow :datetime?

Or maybe just use the first solution with overriding :datetime?

@blackst0ne I prefer not overwriting :datetime as this could break code that expects timestamps without zones (e.g. any Gems we might use). Hence I suggested defining :timestampz as a separate type.

Regarding disallowing :datetime, yes I think that's a good idea.

@yorickpeterse

Rails has such methods:

1. add_timestamps:

def add_timestamps(table_name, options = {})
  options[:null] = false if options[:null].nil?

  add_column table_name, :created_at, :datetime, options
  add_column table_name, :updated_at, :datetime, options
end

2. timestamps:

def timestamps(*args)
  options[:null] = false if options[:null].nil?

  column(:created_at, :datetime, options)
  column(:updated_at, :datetime, options)
end

These two are allowed to be used in migrations. And :datetime data type is hardcoded in these methods.

The question is, what should we do here?

Since you don't like overriding :datetime, we should either redefine that methods to make them use :timestamptz data type or disallow developers to use the methods and add our custom ones.

assigned to @yorickpeterse

assigned to @blackst0ne

assigned to @yorickpeterse

assigned to @blackst0ne

marked as a Work In Progress

changed title from Add database helper 'add_timestamp_with_timezone' to WIP: Add database helpers 'add_timestamps_with_timezone' and 'timestamps_with_timezone'

changed the description

added 1434 commits

• ca9c7abe...acdd1bf7 - 1433 commits from branch master
• 9a40d2da - Add database helpers 'add_timestamps_with_timezone' and 'timestamps_with_timezone'

Compare with previous version

@yorickpeterse, I reworked the MR and updated the description.
Could you take a look, please?

assigned to @yorickpeterse

@blackst0ne I would leave the rubocop disable/all thing for the existing migrations as-is. This ensures that any future rubocop rules don't suddenly fail on those migrations.

assigned to @blackst0ne

@blackst0ne I would leave the rubocop disable/all thing for the existing migrations as-is. This ensures that any future rubocop rules don't suddenly fail on those migrations.

@yorickpeterse

Do you mean to keep rubocop:disable all directive in all migrations where it was before I modified it?
Or to add that directive to all existing migrations which are catched by rubocop?

resolved all discussions

assigned to @yorickpeterse

@blackst0ne

Do you mean to keep rubocop:disable all directive in all migrations where it was before I modified it?

Yes, keep it.

changed milestone to %9.4

@yorickpeterse

Yes, keep it.

Done.

unmarked as a Work In Progress

@blackst0ne There is one spec failing because of what appears to be new migrations using datetime. Also, could you add some documentation to doc/development/migration_style_guide.md? This can just be a small section about using these helpers instead of datetime and such.

assigned to @blackst0ne

added 302 commits

• 0b26ec00...de20057c - 301 commits from branch master
• ff16ae65 - Add database helpers 'add_timestamps_with_timezone' and 'timestamps_with_timezone'

Compare with previous version

@yorickpeterse

1. Rebased on current master.
2. Fixed post-migrate migration.
3. Added documentation.

ps: I renamed indices with indexes. My arguments about this change are in the other discussion above: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11229#note_30222692

assigned to @yorickpeterse

@blackst0ne I left some comments regarding some small grammar changes to the documentation, but the rest looks good to me.

assigned to @blackst0ne

resolved all discussions

@yorickpeterse, all discussions are resolved.

assigned to @yorickpeterse

added 1 commit

• bc00806a - Add database helpers 'add_timestamps_with_timezone' and 'timestamps_with_timezone'

Compare with previous version

approved this merge request

enabled an automatic merge when the pipeline for bc00806a succeeds

merged

mentioned in commit 44b806d0

mentioned in merge request !12292 (merged)