Fixed sqlitemigration foreign key toggling

Fixes #54

CHANGELOG.md

@@ -5,7 +5,18 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
-[Unreleased]: https://github.com/zombiezen/go-sqlite/compare/v0.13.0...main
+[Unreleased]: https://github.com/zombiezen/go-sqlite/compare/v0.13.1...main
+
+## [0.13.1][] - 2023-08-15
+
+Version 0.13.1 fixed a bug with the `sqlitemigration` package.
+
+[0.13.1]: https://github.com/zombiezen/go-sqlite/releases/tag/v0.13.1
+
+### Fixed
+
+- `sqlitemigration` will no longer disable foreign keys during operation
+  ([#54](https://github.com/zombiezen/go-sqlite/issues/54)).
 
 ## [0.13.0][] - 2023-03-28
 

go.work.sum

@@ -1,10 +1,9 @@
 github.com/google/pprof v0.0.0-20221118152302-e6195bd50e26 h1:Xim43kblpZXfIBQsbuBVKCudVG457BR2GZFIz3uw3hQ=
+github.com/google/pprof v0.0.0-20221118152302-e6195bd50e26/go.mod h1:dDKJzRmX4S37WGHujM7tX//fmj1uioxKzKxz3lo4HJo=
 github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 h1:Z9n2FFNUXsshfwJMBgNA0RU6/i7WVaAegv3PtuIHPMs=
+github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51/go.mod h1:CzGEWj7cYgsdH8dAjBGEr58BoE7ScuLd+fwFZ44+/x8=
 github.com/mattn/go-sqlite3 v1.14.16 h1:yOQRA0RpS5PFz/oikGwBEqvAWhWg5ufRz4ETLjwpU1Y=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
-github.com/google/pprof v0.0.0-20221118152302-e6195bd50e26 h1:Xim43kblpZXfIBQsbuBVKCudVG457BR2GZFIz3uw3hQ=
-github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 h1:Z9n2FFNUXsshfwJMBgNA0RU6/i7WVaAegv3PtuIHPMs=
-github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY=
 golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc=
 golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4=
@@ -22,19 +21,19 @@ golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ=
 golang.org/x/text v0.7.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8=
 golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc=
 lukechampine.com/uint128 v1.2.0 h1:mBi/5l91vocEN8otkC5bDLhi2KdCticRiwbdB0O+rjI=
+lukechampine.com/uint128 v1.2.0/go.mod h1:c4eWIwlEGaxC/+H1VguhU4PHXNWDCDMUlWdIWl2j1gk=
 modernc.org/cc/v3 v3.40.0 h1:P3g79IUS/93SYhtoeaHW+kRCIrYaxJ27MFPv+7kaTOw=
+modernc.org/cc/v3 v3.40.0/go.mod h1:/bTg4dnWkSXowUO6ssQKnOV0yMVxDYNIsIrzqTFDGH0=
 modernc.org/ccgo/v3 v3.16.13 h1:Mkgdzl46i5F/CNR/Kj80Ri59hC8TKAhZrYSaqvkwzUw=
+modernc.org/ccgo/v3 v3.16.13/go.mod h1:2Quk+5YgpImhPjv2Qsob1DnZ/4som1lJTodubIcoUkY=
 modernc.org/httpfs v1.0.6 h1:AAgIpFZRXuYnkjftxTAZwMIiwEqAfk8aVB2/oA6nAeM=
+modernc.org/httpfs v1.0.6/go.mod h1:7dosgurJGp0sPaRanU53W4xZYKh14wfzX420oZADeHM=
 modernc.org/opt v0.1.3 h1:3XOZf2yznlhC+ibLltsDGzABUGVx8J6pnFMS3E4dcq4=
+modernc.org/opt v0.1.3/go.mod h1:WdSiB5evDcignE70guQKxYUl14mgWtbClRi5wmkkTX0=
 modernc.org/strutil v1.1.3 h1:fNMm+oJklMGYfU9Ylcywl0CO5O6nTfaowNsh2wpPjzY=
-modernc.org/token v1.0.1 h1:A3qvTqOwexpfZZeyI0FeGPDlSWX5pjZu9hF4lU+EKWg=
-modernc.org/z v1.7.0 h1:xkDw/KepgEjeizO2sNco+hqYkU12taxQFqPEmgm1GWE=
-lukechampine.com/uint128 v1.2.0 h1:mBi/5l91vocEN8otkC5bDLhi2KdCticRiwbdB0O+rjI=
-modernc.org/cc/v3 v3.40.0 h1:P3g79IUS/93SYhtoeaHW+kRCIrYaxJ27MFPv+7kaTOw=
-modernc.org/ccgo/v3 v3.16.13 h1:Mkgdzl46i5F/CNR/Kj80Ri59hC8TKAhZrYSaqvkwzUw=
-modernc.org/httpfs v1.0.6 h1:AAgIpFZRXuYnkjftxTAZwMIiwEqAfk8aVB2/oA6nAeM=
-modernc.org/opt v0.1.3 h1:3XOZf2yznlhC+ibLltsDGzABUGVx8J6pnFMS3E4dcq4=
-modernc.org/strutil v1.1.3 h1:fNMm+oJklMGYfU9Ylcywl0CO5O6nTfaowNsh2wpPjzY=
+modernc.org/strutil v1.1.3/go.mod h1:MEHNA7PdEnEwLvspRMtWTNnp2nnyvMfkimT1NKNAGbw=
 modernc.org/tcl v1.15.1 h1:mOQwiEK4p7HruMZcwKTZPw/aqtGM4aY00uzWhlKKYws=
 modernc.org/token v1.0.1 h1:A3qvTqOwexpfZZeyI0FeGPDlSWX5pjZu9hF4lU+EKWg=
+modernc.org/token v1.0.1/go.mod h1:UGzOrNV1mAFSEB63lOFHIpNRUVMvYTc6yu1SMY/XTDM=
 modernc.org/z v1.7.0 h1:xkDw/KepgEjeizO2sNco+hqYkU12taxQFqPEmgm1GWE=
+modernc.org/z v1.7.0/go.mod h1:hVdgNMh8ggTuRG1rGU8x+xGRFfiQUIAw0ZqlPy8+HyQ=

sqlite_test.go

@@ -948,6 +948,43 @@ func TestSerialize(t *testing.T) {
 	}
 }
 
+func TestForeignKey(t *testing.T) {
+	c, err := sqlite.OpenConn(":memory:")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() {
+		if err := c.Close(); err != nil {
+			t.Error(err)
+		}
+	}()
+
+	err = sqlitex.ExecuteTransient(c, `PRAGMA foreign_keys = on;`, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = sqlitex.ExecuteScript(c, `CREATE TABLE artist(
+  artistid    INTEGER PRIMARY KEY,
+  artistname  TEXT
+);
+CREATE TABLE track(
+  trackid     INTEGER,
+  trackname   TEXT,
+  trackartist INTEGER,
+  FOREIGN KEY(trackartist) REFERENCES artist(artistid)
+);`, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = sqlitex.ExecuteTransient(c, `INSERT INTO track VALUES(14, 'Mr. Bojangles', 3);`, nil)
+	if err == nil {
+		t.Fatal("No error from breaking foreign key")
+	} else {
+		t.Log("Got (intentional) error:", err)
+	}
+}
+
 func TestMain(m *testing.M) {
 	_ = libc.Environ() // Forces libc.SetEnviron; fixes memory accounting balance for environ(7).
 	libc.MemAuditStart()

sqlitemigration/sqlitemigration.go

@@ -287,13 +287,7 @@ func Migrate(ctx context.Context, conn *sqlite.Conn, schema Schema) error {
 func migrateDB(ctx context.Context, conn *sqlite.Conn, schema Schema, onStart SignalFunc) error {
 	defer conn.SetInterrupt(conn.SetInterrupt(ctx.Done()))
 
-	userVersionStmt, _, err := conn.PrepareTransient("PRAGMA user_version;")
-	if err != nil {
-		return fmt.Errorf("migrate database: %w", err)
-	}
-	defer userVersionStmt.Finalize()
-
-	schemaVersion, err := ensureAppID(conn, schema.AppID, userVersionStmt)
+	schemaVersion, err := ensureAppID(conn, schema.AppID)
 	if err != nil {
 		return fmt.Errorf("migrate database: %w", err)
 	}
@@ -303,26 +297,13 @@ func migrateDB(ctx context.Context, conn *sqlite.Conn, schema Schema, onStart Si
 	var foreignKeysEnabled bool
 	err = sqlitex.ExecuteTransient(conn, "PRAGMA foreign_keys;", &sqlitex.ExecOptions{
 		ResultFunc: func(stmt *sqlite.Stmt) error {
-			foreignKeysEnabled = stmt.ColumnInt(0) != 0
+			foreignKeysEnabled = stmt.ColumnBool(0)
 			return nil
 		},
 	})
 	if err != nil {
 		return fmt.Errorf("migrate database: %w", err)
 	}
-	var fkOnStmt, fkOffStmt *sqlite.Stmt
-	if foreignKeysEnabled {
-		fkOnStmt, _, err = conn.PrepareTransient("PRAGMA foreign_keys = on;")
-		if err != nil {
-			return fmt.Errorf("migrate database: %w", err)
-		}
-		defer fkOnStmt.Finalize()
-		fkOffStmt, _, err = conn.PrepareTransient("PRAGMA foreign_keys = off;")
-		if err != nil {
-			return fmt.Errorf("migrate database: %w", err)
-		}
-		defer fkOffStmt.Finalize()
-	}
 
 	beginStmt, _, err := conn.PrepareTransient("BEGIN IMMEDIATE;")
 	if err != nil {
@@ -334,27 +315,24 @@ func migrateDB(ctx context.Context, conn *sqlite.Conn, schema Schema, onStart Si
 		return fmt.Errorf("migrate database: %w", err)
 	}
 	defer commitStmt.Finalize()
-	for ; schemaVersion < len(schema.Migrations); schemaVersion++ {
+	for ; int(schemaVersion) < len(schema.Migrations); schemaVersion++ {
 		migration := schema.Migrations[schemaVersion]
 		disableFKs := foreignKeysEnabled &&
-			schemaVersion < len(schema.MigrationOptions) &&
+			int(schemaVersion) < len(schema.MigrationOptions) &&
 			schema.MigrationOptions[schemaVersion] != nil &&
 			schema.MigrationOptions[schemaVersion].DisableForeignKeys
 		if disableFKs {
-			if err := stepAndReset(fkOffStmt); err != nil {
+			// Do not try to optimize by preparing this PRAGMA statement ahead of time.
+			if err := sqlitex.ExecuteTransient(conn, "PRAGMA foreign_keys = off;", nil); err != nil {
 				return fmt.Errorf("migrate database: disable foreign keys: %w", err)
 			}
 		}
 
 		if err := stepAndReset(beginStmt); err != nil {
 			return fmt.Errorf("migrate database: apply migrations[%d]: %w", schemaVersion, err)
 		}
-		if _, err := userVersionStmt.Step(); err != nil {
-			rollback(conn)
-			return fmt.Errorf("migrate database: %w", err)
-		}
-		actualSchemaVersion := userVersionStmt.ColumnInt(0)
-		if err := userVersionStmt.Reset(); err != nil {
+		actualSchemaVersion, err := userVersion(conn)
+		if err != nil {
 			rollback(conn)
 			return fmt.Errorf("migrate database: %w", err)
 		}
@@ -365,12 +343,12 @@ func migrateDB(ctx context.Context, conn *sqlite.Conn, schema Schema, onStart Si
 			continue
 		}
 
-		err := sqlitex.ExecScript(conn, fmt.Sprintf("%s;\nPRAGMA user_version = %d;\n", migration, schemaVersion+1))
+		err = sqlitex.ExecScript(conn, fmt.Sprintf("%s;\nPRAGMA user_version = %d;\n", migration, schemaVersion+1))
 		if err != nil {
 			rollback(conn)
 			return fmt.Errorf("migrate database: apply migrations[%d]: %w", schemaVersion, err)
 		}
-		if schemaVersion == len(schema.Migrations)-1 && schema.RepeatableMigration != "" {
+		if int(schemaVersion) == len(schema.Migrations)-1 && schema.RepeatableMigration != "" {
 			if err := sqlitex.ExecScript(conn, schema.RepeatableMigration); err != nil {
 				rollback(conn)
 				return fmt.Errorf("migrate database: apply repeatable migration: %w", err)
@@ -382,22 +360,36 @@ func migrateDB(ctx context.Context, conn *sqlite.Conn, schema Schema, onStart Si
 			return fmt.Errorf("migrate database: apply migrations[%d]: %w", schemaVersion, err)
 		}
 		if disableFKs {
-			if err := stepAndReset(fkOnStmt); err != nil {
+			if err := sqlitex.ExecuteTransient(conn, "PRAGMA foreign_keys = on;", nil); err != nil {
 				return fmt.Errorf("migrate database: reenable foreign keys: %w", err)
 			}
 		}
 	}
 	return nil
 }
 
+func userVersion(conn *sqlite.Conn) (int32, error) {
+	var version int32
+	err := sqlitex.ExecuteTransient(conn, "PRAGMA user_version;", &sqlitex.ExecOptions{
+		ResultFunc: func(stmt *sqlite.Stmt) error {
+			version = stmt.ColumnInt32(0)
+			return nil
+		},
+	})
+	if err != nil {
+		return 0, fmt.Errorf("get database user_version: %w", err)
+	}
+	return version, nil
+}
+
 func rollback(conn *sqlite.Conn) {
 	if conn.AutocommitEnabled() {
 		return
 	}
 	sqlitex.ExecuteTransient(conn, "ROLLBACK;", nil)
 }
 
-func ensureAppID(conn *sqlite.Conn, wantAppID int32, userVersionStmt *sqlite.Stmt) (schemaVersion int, err error) {
+func ensureAppID(conn *sqlite.Conn, wantAppID int32) (schemaVersion int32, err error) {
 	defer sqlitex.Save(conn)(&err)
 
 	var hasSchema bool
@@ -423,11 +415,8 @@ func ensureAppID(conn *sqlite.Conn, wantAppID int32, userVersionStmt *sqlite.Stm
 	if dbAppID != wantAppID && !(dbAppID == 0 && !hasSchema) {
 		return 0, fmt.Errorf("database application_id = %#x (expected %#x)", dbAppID, wantAppID)
 	}
-	if _, err := userVersionStmt.Step(); err != nil {
-		return 0, err
-	}
-	schemaVersion = userVersionStmt.ColumnInt(0)
-	if err := userVersionStmt.Reset(); err != nil {
+	schemaVersion, err = userVersion(conn)
+	if err != nil {
 		return 0, err
 	}
 	// Using Sprintf because PRAGMAs don't permit arbitrary expressions, and thus

sqlitemigration/sqlitemigration_test.go

@@ -844,6 +844,79 @@ func TestPool(t *testing.T) {
 			t.Error("Foreign keys were left disabled after migration")
 		}
 	})
+
+	t.Run("NoTouchForeignKeys", func(t *testing.T) {
+		schema := Schema{
+			AppID: 0xedbeef,
+			Migrations: []string{
+				`create table foo ( foreign_keys_enabled bool ); insert into foo values ((select * from pragma_foreign_keys()));`,
+			},
+		}
+		pool := NewPool(filepath.Join(t.TempDir(), "no-touch-foreign-keys.db"), schema, Options{
+			Flags: sqlite.OpenReadWrite | sqlite.OpenCreate,
+			PrepareConn: func(conn *sqlite.Conn) error {
+				return sqlitex.ExecuteTransient(conn, "PRAGMA foreign_keys = on;", nil)
+			},
+		})
+		defer func() {
+			if err := pool.Close(); err != nil {
+				t.Error("pool.Close:", err)
+			}
+		}()
+		conn, err := pool.Get(ctx)
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer pool.Put(conn)
+		duringMigration, err := sqlitex.ResultBool(conn.Prep("select foreign_keys_enabled from foo;"))
+		if err != nil {
+			t.Error(err)
+		} else if !duringMigration {
+			t.Error("Foreign keys were disabled during migration")
+		}
+		afterMigration, err := sqlitex.ResultBool(conn.Prep("PRAGMA foreign_keys;"))
+		if err != nil {
+			t.Error(err)
+		} else if !afterMigration {
+			t.Error("Foreign keys were disabled after migration")
+		}
+	})
+}
+
+func TestMigrate(t *testing.T) {
+	t.Run("NoTouchForeignKeys", func(t *testing.T) {
+		conn, err := sqlite.OpenConn(filepath.Join(t.TempDir(), "no-touch-foreign-keys.db"), sqlite.OpenReadWrite, sqlite.OpenCreate)
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer conn.Close()
+		err = sqlitex.ExecuteTransient(conn, `PRAGMA foreign_keys = on;`, nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		schema := Schema{
+			AppID: 0xedbeef,
+			Migrations: []string{
+				`create table foo ( foreign_keys_enabled bool ); insert into foo values ((select * from pragma_foreign_keys()));`,
+			},
+		}
+		if err := Migrate(context.Background(), conn, schema); err != nil {
+			t.Error(err)
+		}
+		duringMigration, err := sqlitex.ResultBool(conn.Prep("select foreign_keys_enabled from foo;"))
+		if err != nil {
+			t.Error(err)
+		} else if !duringMigration {
+			t.Error("Foreign keys were disabled during migration")
+		}
+		afterMigration, err := sqlitex.ResultBool(conn.Prep("PRAGMA foreign_keys;"))
+		if err != nil {
+			t.Error(err)
+		} else if !afterMigration {
+			t.Error("Foreign keys were disabled after migration")
+		}
+	})
 }
 
 // withTestConn makes an independent connection to the given database.