Skip to content

Commit 5e56da8

Browse files
crepererumcrepererum
authored
fix: ensure migration progress is not lost for PG, mysql and sqlite (#1991)
* fix: ensure migration progress is not lost for PG Fixes #1966. * fix: ensure migration progress is not lost for sqlite This is similar to #1966. * fix: ensure reverse migration progress is not lost for PG See #1966. * fix: ensure reverse migration progress is not lost for sqlite See #1966. * fix: ensure migration progress is not lost for mysql This is similar to #1966. * fix: ensure reverse migration progress is not lost for mysql See #1966. * test: check migration type as well * test: extend migrations testing * fix: work around MySQL implicit commits * refactor: simplify migration testing
1 parent ddffaa7 commit 5e56da8

35 files changed

+612
-50
lines changed

‎Cargo.toml

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,11 @@ name = "sqlite-test-attr"
212212
path = "tests/sqlite/test-attr.rs"
213213
required-features = ["sqlite", "macros", "migrate"]
214214

215+
[[test]]
216+
name = "sqlite-migrate"
217+
path = "tests/sqlite/migrate.rs"
218+
required-features = ["sqlite", "macros", "migrate"]
219+
215220
#
216221
# MySQL
217222
#
@@ -241,6 +246,11 @@ name = "mysql-test-attr"
241246
path = "tests/mysql/test-attr.rs"
242247
required-features = ["mysql", "macros", "migrate"]
243248

249+
[[test]]
250+
name = "mysql-migrate"
251+
path = "tests/mysql/migrate.rs"
252+
required-features = ["mysql", "macros", "migrate"]
253+
244254
#
245255
# PostgreSQL
246256
#
@@ -275,6 +285,11 @@ name = "postgres-test-attr"
275285
path = "tests/postgres/test-attr.rs"
276286
required-features = ["postgres", "macros", "migrate"]
277287

288+
[[test]]
289+
name = "postgres-migrate"
290+
path = "tests/postgres/migrate.rs"
291+
required-features = ["postgres", "macros", "migrate"]
292+
278293
#
279294
# Microsoft SQL Server (MSSQL)
280295
#

‎sqlx-core/src/migrate/migration_type.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/// Migration Type represents the type of migration
2-
#[derive(Debug, Copy, Clone)]
2+
#[derive(Debug, Copy, Clone, PartialEq)]
33
pub enum MigrationType {
44
/// Simple migration are single file migrations with no up / down queries
55
Simple,

‎sqlx-core/src/mysql/migrate.rs

Lines changed: 73 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use crate::connection::ConnectOptions;
1+
use crate::connection::{ConnectOptions, Connection};
22
use crate::error::Error;
33
use crate::executor::Executor;
44
use crate::migrate::MigrateError;
@@ -209,29 +209,67 @@ CREATE TABLE IF NOT EXISTS _sqlx_migrations (
209209
migration: &'m Migration,
210210
) -> BoxFuture<'m, Result<Duration, MigrateError>> {
211211
Box::pin(async move {
212+
// Use a single transaction for the actual migration script and the essential bookeeping so we never
213+
// execute migrations twice. See https://github.com/launchbadge/sqlx/issues/1966.
214+
// The `execution_time` however can only be measured for the whole transaction. This value _only_ exists for
215+
// data lineage and debugging reasons, so it is not super important if it is lost. So we initialize it to -1
216+
// and update it once the actual transaction completed.
217+
let mut tx = self.begin().await?;
212218
let start = Instant::now();
213219

214-
let res = self.execute(&*migration.sql).await;
215-
216-
let elapsed = start.elapsed();
217-
220+
// For MySQL we cannot really isolate migrations due to implicit commits caused by table modification, see
221+
// https://dev.mysql.com/doc/refman/8.0/en/implicit-commit.html
222+
//
223+
// To somewhat try to detect this, we first insert the migration into the migration table with
224+
// `success=FALSE` and later modify the flag.
225+
//
218226
// language=MySQL
219227
let _ = query(
220228
r#"
221229
INSERT INTO _sqlx_migrations ( version, description, success, checksum, execution_time )
222-
VALUES ( ?, ?, ?, ?, ? )
230+
VALUES ( ?, ?, FALSE, ?, -1 )
223231
"#,
224232
)
225233
.bind(migration.version)
226234
.bind(&*migration.description)
227-
.bind(res.is_ok())
228235
.bind(&*migration.checksum)
236+
.execute(&mut tx)
237+
.await?;
238+
239+
let _ = tx.execute(&*migration.sql).await?;
240+
241+
// language=MySQL
242+
let _ = query(
243+
r#"
244+
UPDATE _sqlx_migrations
245+
SET success = TRUE
246+
WHERE version = ?
247+
"#,
248+
)
249+
.bind(migration.version)
250+
.execute(&mut tx)
251+
.await?;
252+
253+
tx.commit().await?;
254+
255+
// Update `elapsed_time`.
256+
// NOTE: The process may disconnect/die at this point, so the elapsed time value might be lost. We accept
257+
// this small risk since this value is not super important.
258+
259+
let elapsed = start.elapsed();
260+
261+
let _ = query(
262+
r#"
263+
UPDATE _sqlx_migrations
264+
SET execution_time = ?
265+
WHERE version = ?
266+
"#,
267+
)
229268
.bind(elapsed.as_nanos() as i64)
269+
.bind(migration.version)
230270
.execute(self)
231271
.await?;
232272

233-
res?;
234-
235273
Ok(elapsed)
236274
})
237275
}
@@ -241,18 +279,41 @@ CREATE TABLE IF NOT EXISTS _sqlx_migrations (
241279
migration: &'m Migration,
242280
) -> BoxFuture<'m, Result<Duration, MigrateError>> {
243281
Box::pin(async move {
282+
// Use a single transaction for the actual migration script and the essential bookeeping so we never
283+
// execute migrations twice. See https://github.com/launchbadge/sqlx/issues/1966.
284+
let mut tx = self.begin().await?;
244285
let start = Instant::now();
245286

246-
self.execute(&*migration.sql).await?;
287+
// For MySQL we cannot really isolate migrations due to implicit commits caused by table modification, see
288+
// https://dev.mysql.com/doc/refman/8.0/en/implicit-commit.html
289+
//
290+
// To somewhat try to detect this, we first insert the migration into the migration table with
291+
// `success=FALSE` and later remove the migration altogether.
292+
//
293+
// language=MySQL
294+
let _ = query(
295+
r#"
296+
UPDATE _sqlx_migrations
297+
SET success = FALSE
298+
WHERE version = ?
299+
"#,
300+
)
301+
.bind(migration.version)
302+
.execute(&mut tx)
303+
.await?;
247304

248-
let elapsed = start.elapsed();
305+
tx.execute(&*migration.sql).await?;
249306

250307
// language=SQL
251308
let _ = query(r#"DELETE FROM _sqlx_migrations WHERE version = ?"#)
252309
.bind(migration.version)
253-
.execute(self)
310+
.execute(&mut tx)
254311
.await?;
255312

313+
tx.commit().await?;
314+
315+
let elapsed = start.elapsed();
316+
256317
Ok(elapsed)
257318
})
258319
}

‎sqlx-core/src/postgres/migrate.rs

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -222,23 +222,44 @@ CREATE TABLE IF NOT EXISTS _sqlx_migrations (
222222
let mut tx = self.begin().await?;
223223
let start = Instant::now();
224224

225+
// Use a single transaction for the actual migration script and the essential bookeeping so we never
226+
// execute migrations twice. See https://github.com/launchbadge/sqlx/issues/1966.
227+
// The `execution_time` however can only be measured for the whole transaction. This value _only_ exists for
228+
// data lineage and debugging reasons, so it is not super important if it is lost. So we initialize it to -1
229+
// and update it once the actual transaction completed.
225230
let _ = tx.execute(&*migration.sql).await?;
226231

227-
tx.commit().await?;
228-
229-
let elapsed = start.elapsed();
230-
231232
// language=SQL
232233
let _ = query(
233234
r#"
234235
INSERT INTO _sqlx_migrations ( version, description, success, checksum, execution_time )
235-
VALUES ( $1, $2, TRUE, $3, $4 )
236+
VALUES ( $1, $2, TRUE, $3, -1 )
236237
"#,
237238
)
238239
.bind(migration.version)
239240
.bind(&*migration.description)
240241
.bind(&*migration.checksum)
242+
.execute(&mut tx)
243+
.await?;
244+
245+
tx.commit().await?;
246+
247+
// Update `elapsed_time`.
248+
// NOTE: The process may disconnect/die at this point, so the elapsed time value might be lost. We accept
249+
// this small risk since this value is not super important.
250+
251+
let elapsed = start.elapsed();
252+
253+
// language=SQL
254+
let _ = query(
255+
r#"
256+
UPDATE _sqlx_migrations
257+
SET execution_time = $1
258+
WHERE version = $2
259+
"#,
260+
)
241261
.bind(elapsed.as_nanos() as i64)
262+
.bind(migration.version)
242263
.execute(self)
243264
.await?;
244265

@@ -251,21 +272,23 @@ CREATE TABLE IF NOT EXISTS _sqlx_migrations (
251272
migration: &'m Migration,
252273
) -> BoxFuture<'m, Result<Duration, MigrateError>> {
253274
Box::pin(async move {
275+
// Use a single transaction for the actual migration script and the essential bookeeping so we never
276+
// execute migrations twice. See https://github.com/launchbadge/sqlx/issues/1966.
254277
let mut tx = self.begin().await?;
255278
let start = Instant::now();
256279

257280
let _ = tx.execute(&*migration.sql).await?;
258281

259-
tx.commit().await?;
260-
261-
let elapsed = start.elapsed();
262-
263282
// language=SQL
264283
let _ = query(r#"DELETE FROM _sqlx_migrations WHERE version = $1"#)
265284
.bind(migration.version)
266-
.execute(self)
285+
.execute(&mut tx)
267286
.await?;
268287

288+
tx.commit().await?;
289+
290+
let elapsed = start.elapsed();
291+
269292
Ok(elapsed)
270293
})
271294
}

‎sqlx-core/src/sqlite/migrate.rs

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -173,23 +173,44 @@ CREATE TABLE IF NOT EXISTS _sqlx_migrations (
173173
let mut tx = self.begin().await?;
174174
let start = Instant::now();
175175

176+
// Use a single transaction for the actual migration script and the essential bookeeping so we never
177+
// execute migrations twice. See https://github.com/launchbadge/sqlx/issues/1966.
178+
// The `execution_time` however can only be measured for the whole transaction. This value _only_ exists for
179+
// data lineage and debugging reasons, so it is not super important if it is lost. So we initialize it to -1
180+
// and update it once the actual transaction completed.
176181
let _ = tx.execute(&*migration.sql).await?;
177182

178-
tx.commit().await?;
179-
180-
let elapsed = start.elapsed();
181-
182183
// language=SQL
183184
let _ = query(
184185
r#"
185186
INSERT INTO _sqlx_migrations ( version, description, success, checksum, execution_time )
186-
VALUES ( ?1, ?2, TRUE, ?3, ?4 )
187+
VALUES ( ?1, ?2, TRUE, ?3, -1 )
187188
"#,
188189
)
189190
.bind(migration.version)
190191
.bind(&*migration.description)
191192
.bind(&*migration.checksum)
193+
.execute(&mut tx)
194+
.await?;
195+
196+
tx.commit().await?;
197+
198+
// Update `elapsed_time`.
199+
// NOTE: The process may disconnect/die at this point, so the elapsed time value might be lost. We accept
200+
// this small risk since this value is not super important.
201+
202+
let elapsed = start.elapsed();
203+
204+
// language=SQL
205+
let _ = query(
206+
r#"
207+
UPDATE _sqlx_migrations
208+
SET execution_time = ?1
209+
WHERE version = ?2
210+
"#,
211+
)
192212
.bind(elapsed.as_nanos() as i64)
213+
.bind(migration.version)
193214
.execute(self)
194215
.await?;
195216

@@ -202,21 +223,23 @@ CREATE TABLE IF NOT EXISTS _sqlx_migrations (
202223
migration: &'m Migration,
203224
) -> BoxFuture<'m, Result<Duration, MigrateError>> {
204225
Box::pin(async move {
226+
// Use a single transaction for the actual migration script and the essential bookeeping so we never
227+
// execute migrations twice. See https://github.com/launchbadge/sqlx/issues/1966.
205228
let mut tx = self.begin().await?;
206229
let start = Instant::now();
207230

208231
let _ = tx.execute(&*migration.sql).await?;
209232

210-
tx.commit().await?;
211-
212-
let elapsed = start.elapsed();
213-
214233
// language=SQL
215234
let _ = query(r#"DELETE FROM _sqlx_migrations WHERE version = ?1"#)
216235
.bind(migration.version)
217-
.execute(self)
236+
.execute(&mut tx)
218237
.await?;
219238

239+
tx.commit().await?;
240+
241+
let elapsed = start.elapsed();
242+
220243
Ok(elapsed)
221244
})
222245
}

‎tests/migrate/macro.rs

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,29 @@
11
use sqlx::migrate::Migrator;
22
use std::path::Path;
33

4-
static EMBEDDED: Migrator = sqlx::migrate!("tests/migrate/migrations");
4+
static EMBEDDED_SIMPLE: Migrator = sqlx::migrate!("tests/migrate/migrations_simple");
5+
static EMBEDDED_REVERSIBLE: Migrator = sqlx::migrate!("tests/migrate/migrations_reversible");
56

67
#[sqlx_macros::test]
78
async fn same_output() -> anyhow::Result<()> {
8-
let runtime = Migrator::new(Path::new("tests/migrate/migrations")).await?;
9+
let runtime_simple = Migrator::new(Path::new("tests/migrate/migrations_simple")).await?;
10+
let runtime_reversible =
11+
Migrator::new(Path::new("tests/migrate/migrations_reversible")).await?;
912

10-
assert_eq!(runtime.migrations.len(), EMBEDDED.migrations.len());
13+
assert_same(&EMBEDDED_SIMPLE, &runtime_simple);
14+
assert_same(&EMBEDDED_REVERSIBLE, &runtime_reversible);
1115

12-
for (e, r) in EMBEDDED.iter().zip(runtime.iter()) {
16+
Ok(())
17+
}
18+
19+
fn assert_same(embedded: &Migrator, runtime: &Migrator) {
20+
assert_eq!(runtime.migrations.len(), embedded.migrations.len());
21+
22+
for (e, r) in embedded.iter().zip(runtime.iter()) {
1323
assert_eq!(e.version, r.version);
1424
assert_eq!(e.description, r.description);
25+
assert_eq!(e.migration_type, r.migration_type);
1526
assert_eq!(e.sql, r.sql);
1627
assert_eq!(e.checksum, r.checksum);
1728
}
18-
19-
Ok(())
2029
}

‎tests/migrate/migrations/20200723212833_tweet.sql

Lines changed: 0 additions & 6 deletions
This file was deleted.

‎tests/migrate/migrations/20200723212841_accounts.sql

Lines changed: 0 additions & 5 deletions
This file was deleted.

‎tests/migrate/migrations_reversible/20220721124650_add_table.down.sql

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
DROP TABLE migrations_reversible_test;

‎tests/migrate/migrations_reversible/20220721124650_add_table.up.sql

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
CREATE TABLE migrations_reversible_test (
2+
some_id BIGINT NOT NULL PRIMARY KEY,
3+
some_payload BIGINT NOT NUll
4+
);
5+
6+
INSERT INTO migrations_reversible_test (some_id, some_payload)
7+
VALUES (1, 100);

‎tests/migrate/migrations_reversible/20220721125033_modify_column.down.sql

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
UPDATE migrations_reversible_test
2+
SET some_payload = some_payload - 1;

‎tests/migrate/migrations_reversible/20220721125033_modify_column.up.sql

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
UPDATE migrations_reversible_test
2+
SET some_payload = some_payload + 1;

0 commit comments

Comments
 (0)