Skip to content

Commit

Permalink
pick(19558) indexer: relax migration check (#19563)
Browse files Browse the repository at this point in the history
## Description

It's okay for the list of locally known migrations to be a subset of the
applied migrations, rather than a prefix (The only clear issue is if
there is a migration that has been embedded locally, but that hasn't
been run on the DB).

## Test plan

New unit test:

```
sui-indexer$ cargo nextest run
```

And confirm against the production DB.

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol:
- [ ] Nodes (Validators and Full nodes):
- [ ] Indexer:
- [ ] JSON-RPC:
- [ ] GraphQL:
- [ ] CLI:
- [ ] Rust SDK:
- [ ] REST API:
  • Loading branch information
amnn authored Sep 26, 2024
1 parent 0d3bec8 commit 8945afb
Showing 1 changed file with 48 additions and 20 deletions.
68 changes: 48 additions & 20 deletions crates/sui-indexer/src/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use diesel::r2d2::ConnectionManager;
use diesel::r2d2::{Pool, PooledConnection};
use diesel::PgConnection;
use diesel_migrations::{embed_migrations, EmbeddedMigrations};
use std::collections::BTreeSet;
use std::time::Duration;
use tracing::info;

Expand Down Expand Up @@ -155,27 +156,26 @@ fn check_db_migration_consistency_impl(
// Unfortunately we cannot call applied_migrations() directly on the connection,
// since it implicitly creates the __diesel_schema_migrations table if it doesn't exist,
// which is a write operation that we don't want to do in this function.
let applied_migrations: Vec<MigrationVersion> = __diesel_schema_migrations::table
.select(__diesel_schema_migrations::version)
.order(__diesel_schema_migrations::version.asc())
.load(conn)?;

// We check that the local migrations is a prefix of the applied migrations.
if local_migrations.len() > applied_migrations.len() {
return Err(IndexerError::DbMigrationError(format!(
"The number of local migrations is greater than the number of applied migrations. Local migrations: {:?}, Applied migrations: {:?}",
local_migrations, applied_migrations
)));
let applied_migrations: BTreeSet<MigrationVersion<'_>> = BTreeSet::from_iter(
__diesel_schema_migrations::table
.select(__diesel_schema_migrations::version)
.load(conn)?,
);

// We check that the local migrations is a subset of the applied migrations.
let unapplied_migrations: Vec<_> = local_migrations
.into_iter()
.filter(|m| !applied_migrations.contains(m))
.collect();

if unapplied_migrations.is_empty() {
return Ok(());
}
for (local_migration, applied_migration) in local_migrations.iter().zip(&applied_migrations) {
if local_migration != applied_migration {
return Err(IndexerError::DbMigrationError(format!(
"The next applied migration `{:?}` diverges from the local migration `{:?}`",
applied_migration, local_migration
)));
}
}
Ok(())

Err(IndexerError::DbMigrationError(format!(
"This binary expected the following migrations to have been run, and they were not: {:?}",
unapplied_migrations
)))
}

pub use setup_postgres::{reset_database, run_migrations};
Expand Down Expand Up @@ -314,4 +314,32 @@ mod tests {
// This should pass the consistency check since it's still a prefix.
check_db_migration_consistency_impl(&mut conn, local_migrations).unwrap();
}

#[tokio::test]
async fn db_migration_consistency_subset_test() {
let database = TempDb::new().unwrap();
let pool = ConnectionPool::new(
database.database().url().to_owned(),
ConnectionPoolConfig {
pool_size: 2,
..Default::default()
},
)
.await
.unwrap();

reset_database(pool.dedicated_connection().await.unwrap())
.await
.unwrap();

let migrations: Vec<Box<dyn Migration<Pg>>> = MIGRATIONS.migrations().unwrap();
let mut local_migrations: Vec<_> = migrations.iter().map(|m| m.name().version()).collect();
local_migrations.remove(2);

// Local migrations are missing one record compared to the applied migrations, which should
// still be okay.
check_db_migration_consistency_impl(&mut pool.get().await.unwrap(), local_migrations)
.await
.unwrap();
}
}

0 comments on commit 8945afb

Please sign in to comment.