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.

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

sqlitex.Save may to ROLLBACK to nonexistent SAVEPOINT after SQLITE_INTERRUPT #73

Closed
AdamSLevy opened this issue Oct 7, 2019 · 0 comments · Fixed by #74
Closed

sqlitex.Save may to ROLLBACK to nonexistent SAVEPOINT after SQLITE_INTERRUPT #73

AdamSLevy opened this issue Oct 7, 2019 · 0 comments · Fixed by #74

Comments

@AdamSLevy
Copy link
Collaborator

AdamSLevy commented Oct 7, 2019

When a sqlite.Conn has an interrupt channel that is closed, any running queries are interrupted and new queries will not be possible. If a query was running when the interrupt occurred, then the SAVEPOINT will have automatically been rolled back. When the releaseFn then goes to ROLLBACK to that SAVEPOINT, a SQLITE_ERROR: no such savepoint error is returned, which then causes a panic inside the releaseFn.

This is not caught by the test suite for sqlitex because in the TestInterruptRollback, the context is canceled between queries, so no query is actually interrupted, and so the SAVEPOINTs remain, and the releaseFns take care of the rollback without issue.

ctx, cancel := context.WithCancel(context.Background())
conn.SetInterrupt(ctx.Done())
releaseFn1 := Save(conn)
if err := Exec(conn, `INSERT INTO t (c) VALUES (2);`, nil); err != nil {
t.Fatal(err)
}
releaseFn2 := Save(conn)
if err := Exec(conn, `INSERT INTO t (c) VALUES (3);`, nil); err != nil {
t.Fatal(err)
}
cancel()

Below is an example of such an error from an application that I'm working on. In this case, the DELETE FROM statement was interrupted by me closing the program which caused the context to be canceled. Since a query was actually interrupted, all of the two SAVEPOINTS were rolled back automatically before the releaseFn(s) attempted to.

panic: sqlite.Stmt.Step: SQLITE_INTERRUPT (DELETE FROM "nf_tokens";
                DELETE FROM "nf_token_transactions";
                DELETE FROM "eblocks";
                DELETE FROM "entries";
                UPDATE "metadata" SET ("init_entry_id", "num_issued") = (NULL, NULL);)
	sqlite.Exec: Stmt.Step: SQLITE_ERROR: no such savepoint: crawshaw.io/sqlite/sqlitex.ExecScript (ROLLBACK TO "crawshaw.io/sqlite/sqlitex.ExecScript";) [recovered]
	panic: sqlite.Exec: Stmt.Step: SQLITE_ERROR: no such savepoint: github.com/Factom-Asset-Tokens/fatd/db.Chain.Validate (ROLLBACK TO "github.com/Factom-Asset-Tokens/fatd/db.Chain.Validate";)

goroutine 1 [running]:
crawshaw.io/sqlite/sqlitex.savepoint.func1(0xc00010d890)
	/home/aslevy/go/pkg/mod/github.com/!adam!s!levy/sqlite@v0.1.3-0.20191006235146-265abd16c79e/sqlitex/savepoint.go:106 +0x52c
panic(0x8e7b60, 0xc00010daa0)
	/usr/lib/go/src/runtime/panic.go:679 +0x1b2
crawshaw.io/sqlite/sqlitex.savepoint.func1(0xc00010d9a0)
	/home/aslevy/go/pkg/mod/github.com/!adam!s!levy/sqlite@v0.1.3-0.20191006235146-265abd16c79e/sqlitex/savepoint.go:106 +0x52c
crawshaw.io/sqlite/sqlitex.ExecScript(0xc0001f5a40, 0x9aba11, 0xf1, 0x0, 0x0)
	/home/aslevy/go/pkg/mod/github.com/!adam!s!levy/sqlite@v0.1.3-0.20191006235146-265abd16c79e/sqlitex/exec.go:195 +0x1f6
github.com/Factom-Asset-Tokens/fatd/db.Chain.Validate(0xc00012fee0, 0xc00010b690, 0x9, 0xc00012fe80, 0xc00012fee0, 0xc00012fec0, 0x0, 0xed46e8574, 0xd53aa0, 0x134c8, ...)
	db/validate.go:82 +0x4b7
github.com/Factom-Asset-Tokens/fatd/engine.loadChains(0xa37a20, 0xc000118040, 0x18533, 0x0, 0x0)
	engine/chainmap.go:180 +0xcbb
github.com/Factom-Asset-Tokens/fatd/engine.Start(0xa37a20, 0xc000118040, 0x0)
	engine/engine.go:122 +0x40f
main._main(0x0)
	main.go:62 +0x2d3
main.main()
	main.go:36 +0x22

Although this is using my patched version of this package, the relevant code is the same:

sqlite/sqlitex/savepoint.go

Lines 100 to 111 in 66f853b

// Error path.
// Always run ROLLBACK even if the connection has been interrupted.
oldDoneCh := conn.SetInterrupt(nil)
defer conn.SetInterrupt(oldDoneCh)
err := Exec(conn, fmt.Sprintf("ROLLBACK TO %q;", name), nil)
if err != nil {
panic(orig + err.Error())
}
err = Exec(conn, fmt.Sprintf("RELEASE %q;", name), nil)
if err != nil {
panic(orig + err.Error())
}

The sqlite documentation for sqlite3_interrupt(D) states the following:

An SQL operation that is interrupted will return SQLITE_INTERRUPT. If the interrupted SQL operation is an INSERT, UPDATE, or DELETE that is inside an explicit transaction, then the entire transaction will be rolled back automatically.

https://www.sqlite.org/c3ref/interrupt.html

So if such a query is interrupted, then all SAVEPOINTs will be automatically rolled back. However if no query is running at the time, then the SAVEPOINTs will remain.

The only way to determine if we are in the middle of a transaction is to check the autocommit mode.

If certain kinds of errors occur on a statement within a multi-statement transaction (errors including SQLITE_FULL, SQLITE_IOERR, SQLITE_NOMEM, SQLITE_BUSY, and SQLITE_INTERRUPT) then the transaction might be rolled back automatically. The only way to find out whether SQLite automatically rolled back the transaction after an error is to use this function.

http://www.sqlite.org/c3ref/get_autocommit.html

This bug appears to trace back to Issue #21 where it appears that it was assumed that rollbacks must always explicitly occur after an interrupt.

I believe the correct behavior is to check for autocommit mode before attempting the ROLLBACK/RELEASE.

I will submit a PR for this shortly.

@AdamSLevy AdamSLevy changed the title sqlitex.Save attempts to rollback to nonexistent savepoints after SQLITE_INTERRUPT sqlitex.Save may to ROLLBACK to nonexistent SAVEPOINT after SQLITE_INTERRUPT Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant