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.

Sign up for GitHub

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

Jump to bottom

chore(popx): improve migration performance by removing unnecessary queries #730

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+14 −12
Open

chore(popx): improve migration performance by removing unnecessary queries #730

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions popx/migrator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,18 +94,24 @@ func (m *Migrator) UpTo(ctx context.Context, step int) (applied int, err error)
defer span.End()

c := m.Connection.WithContext(ctx)
mtn := m.sanitizedMigrationTableName(c)

var appliedMigrations []migrationRow
err = c.RawQuery(fmt.Sprintf("SELECT * FROM %s", mtn)).All(&appliedMigrations)
if err != nil && !errIsTableNotFound(err) {
return 0, errors.Wrap(err, "migration up: unable to fetch applied migrations")
}
appliedMigrationsMap := make(map[string]struct{}, len(appliedMigrations))
for _, am := range appliedMigrations {
appliedMigrationsMap[am.Version] = struct{}{}
}

err = m.exec(ctx, func() error {
mtn := m.sanitizedMigrationTableName(c)
mfs := m.Migrations["up"].SortAndFilter(c.Dialect.Name())
for _, mi := range mfs {
l := m.l.WithField("version", mi.Version).WithField("migration_name", mi.Name).WithField("migration_file", mi.Path)

exists, err := c.Where("version = ?", mi.Version).Exists(mtn)
if err != nil {
return errors.Wrapf(err, "problem checking for migration version %s", mi.Version)
}

if exists {
if _, exists := appliedMigrationsMap[mi.Version]; exists {
Copy link
Member

@aeneasr aeneasr Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically we could end up with an outdated view of already applied migrations, for example a migration that is applied 2 times (for whatever reason) or because there are more than one migration workers. Are we OK accepting that risk for the benefit it brings?

Copy link
Member Author

@zepatrik zepatrik Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, theoretically this could be an outdated view.
The benefit will probably be a few hundred ms up to maybe 2s from my testing. This happens a couple of times per test run.
If you think this is a possible source of issues it is probably worth the extra few hundred ms in tests though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed replying to this. I'm not sure to be honest. Migrations should be run as singletons but I guess the question is how do we deal with it if it isn't - so in the case of an operational mistake. Not sure to be honest...

l.Debug("Migration has already been applied, skipping.")
continue
}
Expand All @@ -114,12 +120,8 @@ func (m *Migrator) UpTo(ctx context.Context, step int) (applied int, err error)
l.Debug("Migration has not been applied but it might be a legacy migration, investigating.")

legacyVersion := mi.Version[:14]
exists, err = c.Where("version = ?", legacyVersion).Exists(mtn)
if err != nil {
return errors.Wrapf(err, "problem checking for migration version %s", mi.Version)
}

if exists {
if _, exists := appliedMigrationsMap[legacyVersion]; exists {
l.WithField("legacy_version", legacyVersion).WithField("migration_table", mtn).Debug("Migration has already been applied in a legacy migration run. Updating version in migration table.")
if err := m.isolatedTransaction(ctx, "init-migrate", func(conn *pop.Connection) error {
// We do not want to remove the legacy migration version or subsequent migrations might be applied twice.
Expand Down
Loading