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

Increase max backup size #12602

Merged
merged 2 commits into from Dec 17, 2019
Merged

Increase max backup size #12602

merged 2 commits into from Dec 17, 2019

Conversation

@scd31
Copy link
Contributor

scd31 commented Dec 12, 2019

Changes backup size from integer to bigint. This solves #12572

Stephen
@scd31 scd31 force-pushed the scd31:increase_max_backup_size branch from 7536edc to a58961f Dec 12, 2019
@ThibG
ThibG approved these changes Dec 12, 2019
@@ -653,8 +653,8 @@
create_table "status_pins", force: :cascade do |t|
t.bigint "account_id", null: false
t.bigint "status_id", null: false
t.datetime "created_at", default: -> { "now()" }, null: false
t.datetime "updated_at", default: -> { "now()" }, null: false
t.datetime "created_at", default: -> { "CURRENT_TIMESTAMP" }, null: false

This comment has been minimized.

Copy link
@Gargron

Gargron Dec 15, 2019

Member

What's going on here?

This comment has been minimized.

Copy link
@scd31

scd31 Dec 15, 2019

Author Contributor

Seems that CURRENT_TIMESTAMP was added to Rails 5. I'm not sure why it decided to change in this PR(and I can change it back if needed). This seems like the more rails-y way to do it, as it is database-agnostic.

This comment has been minimized.

Copy link
@Gargron

Gargron Dec 16, 2019

Member

Yes, I'd like to minimize the changes as much as possible. We don't need to be database-agnostic anyway because we use postgres-specific query features.

This comment has been minimized.

Copy link
@scd31

scd31 Dec 16, 2019

Author Contributor

This should be fixed now

@@ -703,30 +703,6 @@
t.index ["tag_id", "status_id"], name: "index_statuses_tags_on_tag_id_and_status_id", unique: true
end

create_table "stream_entries", force: :cascade do |t|

This comment has been minimized.

Copy link
@Gargron

Gargron Dec 15, 2019

Member

This is okay but shouldn't have been there in the first place...

Stephen
@ThibG
ThibG approved these changes Dec 17, 2019
@Gargron Gargron merged commit 3830c0b into tootsuite:master Dec 17, 2019
2 checks passed
2 checks passed
build-and-test Workflow: build-and-test
Details
codeclimate All good!
Details
@zunda

This comment has been minimized.

Copy link
Collaborator

zunda commented Dec 17, 2019

ActiveRecord complains about this migration. Might there be a workaround?

Migrating to IncreaseBackupSize (20191212003415)
== 20191212003415 IncreaseBackupSize: migrating ===============================
rake aborted!
StandardError: An error has occurred, this and all later migrations canceled:

=== Dangerous operation detected #strong_migrations ===

Changing the type of an existing column requires the entire
table and indexes to be rewritten. A safer approach is to:

1. Create a new column
2. Write to both columns
3. Backfill data from the old column to the new column
4. Move reads from the old column to the new column
5. Stop writing to the old column
6. Drop the old column

/app/db/migrate/20191212003415_increase_backup_size.rb:3:in `up'
  :
@scd31

This comment has been minimized.

Copy link
Contributor Author

scd31 commented Dec 17, 2019

@zunda Well that sucks. I don't have time to fix that(Exam season is right now for me) but someone else should submit a PR to fix that. Otherwise I will have a PR up in a few days.

ponapalt added a commit to ponapalt/mastodon that referenced this pull request Dec 17, 2019
@noellabo

This comment has been minimized.

Copy link
Contributor

noellabo commented Dec 17, 2019

Since the number of data in the backups table is usually very small and the target field is not indexed, operations that change the type of this field seem to be safe in practice.

Display the actual number of records and make sure it is small enough.

bin/rails r 'puts Backup.count'

If it looks okay, run migration with SAFETY_ASSURED=1.

SAFETY_ASSURED=1 RAILS_ENV=production bundle exec rails db:migrate

You can also change the migration code.

safety_assured { change_column :backups, :dump_file_size, :bigint }
@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented Dec 17, 2019

Since the schema version was updated in the PR, I assumed db:migrate has been run successfully. It's strange that this warning didn't come up. In practice I think that this table is so small it's okay to update the column live.

There is I believe also a migration helper to do this the safe way.

ponapalt added a commit to ponapalt/mastodon that referenced this pull request Dec 18, 2019
@scd31

This comment has been minimized.

Copy link
Contributor Author

scd31 commented Dec 18, 2019

@Gargron I ran db:migrate, however my database was a clean slate(I had only just installed mastodon for development).
I agree that safety_assured { change_column :backups, :dump_file_size, :bigint } is the way to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.