Skip to content

NoOp DB Migrations to allow for DB backports #23078

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

Closed
wants to merge 3 commits into from

Conversation

techknowlogick
Copy link
Member

@techknowlogick techknowlogick commented Feb 22, 2023

Due to our migrations table only keeping a count of the highest migration that has been run, this means that PRs with migrations cannot be backported as then the migration count would conflict if the administrator ever tries to upgrade to a newer major release.

The purpose of this PR is to add several (15 in this case) no-op migrations, so that the count would be artificially inflated to allow for migration backports.

Pretend Example:
v1.A - max migration = 100
v1.B - migrations start at 101 + 15 noops
An index is added to a large table in v1.B to improve speed of queries, as there are 15 noops this can become migration 101 in v1.A (as when upgrading to v1.B migration 101 would be skipped)

Note: This has a failure case of when a migration may attempted to be run twice on upgrade (the migration that is backported would have a different number, and so on upgrade it would be attempted to be run again), as well as a few others. There are some good suggestions below on improvements to this PR of tracking all migrations that are run to ensure they aren't run in duplicate, as well as a few others.

Alternative: #10625

@techknowlogick techknowlogick added type/miscellaneous skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Feb 22, 2023
@techknowlogick techknowlogick added this to the 1.20.0 milestone Feb 22, 2023
@wxiaoguang
Copy link
Contributor

It will output a lot of logs?

		log.Info("Migration[%d]: %s", v+int64(i), m.Description())

How about just use nil to fill the slice.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 22, 2023
@lunny
Copy link
Member

lunny commented Feb 22, 2023

If I'm not wrong, these migrations are prepared for the future. But I don't think it will work well.

@wxiaoguang
Copy link
Contributor

If I'm not wrong, these migrations are prepared for the future. But I don't think it will work well.

Agree, it needs a complete document to clarify all the cases. Otherwise bugs come ....

@lunny
Copy link
Member

lunny commented Feb 22, 2023

If I'm not wrong, these migrations are prepared for the future. But I don't think it will work well.

Agree, it needs a complete document to clarify all the cases. Otherwise bugs come ....

I think the problems is when user upgrade from an old version which have executed some placeholder migrations, it may encountered problems. Assume users are in v1.19.3 which will add a new column, but for v1.20.0 has no migration for this column but v1.20.3 has, when user upgrade from v1.19.3 to v1.20.0 not v1.20.3, it will encounter problems.

@techknowlogick techknowlogick added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Feb 22, 2023
@techknowlogick
Copy link
Member Author

Blocking per @lunny to ensure this is the correct approach.

@wxiaoguang
Copy link
Contributor

The migration number has finished its job. To make the migration more stable, each migration task should has its own ID and the ID should be saved into database.

Then a migration task in 1.20 can be skipped if the same task has been executed in 1.19.

@lunny
Copy link
Member

lunny commented Feb 22, 2023

The migration number has finished its job. To make the migration more stable, each migration task should has its own ID and the ID should be saved into database.

Then a migration task in 1.20 can be skipped if the same task has been executed in 1.19.

Maybe a UUID generated manually? But it doesn't resolve my concern.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 22, 2023

The migration number has finished its job. To make the migration more stable, each migration task should has its own ID and the ID should be saved into database.
Then a migration task in 1.20 can be skipped if the same task has been executed in 1.19.

Maybe a UUID generated manually? But it doesn't resolve my concern.

Make backported migrations also have the "minor target version" (or use binary build/release date, etc). For example: if a migration backported to 1.19.3 should declare that the next minor version should be not less than 1.20.3 for 1.20.x and not less than 1.21.1 for 1.21.x (suppose 1.22 is under development).

@wxiaoguang
Copy link
Contributor

As the first step of improving the migration system, I think current number approach still works for most cases.

There are 2 kinds of migrations:

  1. idempotent migration (or can be idempotent-like)
    • increase column size
    • add a column (skip it if the column already exists)
    • fix column value which is compatible for all versions
  2. one-time migration
    • change column type to an incompatible one
    • update column value which causes incompatible data between versions
    • rename/drop column

I think most migrations are kind 1. So current simple approach could do its job.

@wolfogre

This comment was marked as resolved.

@techknowlogick

This comment was marked as resolved.

@codecov-commenter

This comment was marked as outdated.

@techknowlogick techknowlogick deleted the noop-migrations branch May 19, 2023 20:53
@GiteaBot GiteaBot removed this from the 1.20.0 milestone May 19, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR type/miscellaneous
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants