Repository: mattn/go-sqlite3
Pull request #1071 (2022-08-05 15:10:54)

Rollback on constraint failure
This fixes an issue where deferred constraints can leave the transaction in an inconsistent state. In the event of a failed deferred constraint, we need to issue a rollback in order to make a transaction startable later.

mattn: @rittneje Do you have an opinion?
rittneje: Given that database/sql will not allow a transaction to be reused once you call `Commit()` (even if it fails), should we just issue a `ROLLBACK` on error, regardless of which specific error it was? The SQLite docs say this:
> If the transaction has already been rolled back automatically by the error response, then the ROLLBACK command will fail with an error, but no harm is caused by this.
joshbuddy: Sounds good to me @rittneje. I've updated the PR and also updated the comment.
rittneje: @joshbuddy LGTM, just one typo.
joshbuddy: @rittneje fixed, thank you
joshbuddy: Looks like some test flakiness?
rittneje: @joshbuddy I think there may be something wrong with `TestQueryer`, and due to an oversight in how it got written, it is not reporting the real issue. Can you update that unit test like so?
```go
for rows.Next() {
    ...
}
if err := rows.Err(); err != nil {
    t.Fatal(err)
}
if n != 3 {
    ...
}
```
joshbuddy: Hey @rittneje, I fixed the underlying test. If you want me to split it off into a separate PR, let me know.
joshbuddy: I've moved the test fixes to https://github.com/mattn/go-sqlite3/pull/1079
codecov-commenter: # [Codecov](https://codecov.io/gh/mattn/go-sqlite3/pull/1071?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattn) Report
> Merging [#1071](https://codecov.io/gh/mattn/go-sqlite3/pull/1071?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattn) (0f9d82e) into [master](https://codecov.io/gh/mattn/go-sqlite3/commit/f92b6bb2a1edf1b47708052ae38b6d392e437761?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattn) (f92b6bb) will **not change** coverage.
> The diff coverage is `100.00%`.

```diff
@@           Coverage Diff           @@
##           master    #1071   +/-   ##
=======================================
  Coverage   45.96%   45.96%           
=======================================
  Files          11       11           
  Lines        1499     1499           
=======================================
  Hits          689      689           
  Misses        670      670           
  Partials      140      140           
```


| [Impacted Files](https://codecov.io/gh/mattn/go-sqlite3/pull/1071?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattn) | Coverage Δ | |
|---|---|---|
| [sqlite3.go](https://codecov.io/gh/mattn/go-sqlite3/pull/1071/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattn#diff-c3FsaXRlMy5nbw==) | `52.64% <100.00%> (ø)` | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattn). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattn)


Patch:
diff --git a/sqlite3.go b/sqlite3.go
index e037857..9c0f4d8 100644
--- a/sqlite3.go
+++ b/sqlite3.go
@@ -494,10 +494,12 @@ func (ai *aggInfo) Done(ctx *C.sqlite3_context) {
 // Commit transaction.
 func (tx *SQLiteTx) Commit() error {
 	_, err := tx.c.exec(context.Background(), "COMMIT", nil)
-	if err != nil && err.(Error).Code == C.SQLITE_BUSY {
-		// sqlite3 will leave the transaction open in this scenario.
+	if err != nil {
+		// sqlite3 may leave the transaction open in this scenario.
 		// However, database/sql considers the transaction complete once we
 		// return from Commit() - we must clean up to honour its semantics.
+		// We don't know if the ROLLBACK is strictly necessary, but according
+		// to sqlite's docs, there is no harm in calling ROLLBACK unnecessarily.
 		tx.c.exec(context.Background(), "ROLLBACK", nil)
 	}
 	return err


Test patch:
diff --git a/sqlite3_test.go b/sqlite3_test.go
index 33d03fd..326361e 100644
--- a/sqlite3_test.go
+++ b/sqlite3_test.go
@@ -248,6 +248,43 @@ func TestForeignKeys(t *testing.T) {
 	}
 }
 
+func TestDeferredForeignKey(t *testing.T) {
+	fname := TempFilename(t)
+	uri := "file:" + fname + "?_foreign_keys=1"
+	db, err := sql.Open("sqlite3", uri)
+	if err != nil {
+		os.Remove(fname)
+		t.Errorf("sql.Open(\"sqlite3\", %q): %v", uri, err)
+	}
+	_, err = db.Exec("CREATE TABLE bar (id INTEGER PRIMARY KEY)")
+	if err != nil {
+		t.Errorf("failed creating tables: %v", err)
+	}
+	_, err = db.Exec("CREATE TABLE foo (bar_id INTEGER, FOREIGN KEY(bar_id) REFERENCES bar(id) DEFERRABLE INITIALLY DEFERRED)")
+	if err != nil {
+		t.Errorf("failed creating tables: %v", err)
+	}
+	tx, err := db.Begin()
+	if err != nil {
+		t.Errorf("Failed to begin transaction: %v", err)
+	}
+	_, err = tx.Exec("INSERT INTO foo (bar_id) VALUES (123)")
+	if err != nil {
+		t.Errorf("Failed to insert row: %v", err)
+	}
+	err = tx.Commit()
+	if err == nil {
+		t.Errorf("Expected an error: %v", err)
+	}
+	_, err = db.Begin()
+	if err != nil {
+		t.Errorf("Failed to begin transaction: %v", err)
+	}
+
+	db.Close()
+	os.Remove(fname)
+}
+
 func TestRecursiveTriggers(t *testing.T) {
 	cases := map[string]bool{
 		"?_recursive_triggers=1": true,