-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Adding public method to reset a prepared statement #1145
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
Adding public method to reset a prepared statement #1145
Conversation
… that clients can reset any prepared statements they keep in memory, which allows for transactions to completely finish. without closing transactions (regardless of commit), the wal file cannot be checkpointed and will grow unbounded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Can you add one line somewhere in documentation to mention the existence of this method?
// create new db on disk in wal mode | ||
let db = try Connection(.uri(path)) | ||
let url = URL(fileURLWithPath: db.description) | ||
XCTAssertEqual(url.lastPathComponent, "SQLite.swift Tests.sqlite3") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary test
let path = "\(NSTemporaryDirectory())/SQLite.swift Tests.sqlite3" | ||
try? FileManager.default.removeItem(atPath: path) | ||
try? FileManager.default.removeItem(atPath: path + "-shm") | ||
try? FileManager.default.removeItem(atPath: path + "-wal") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's better to create new and unique temporary file instead: see temporaryFile()
in Fixtures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need to remove these files since they are created each time
Just pushed up changes to the test, as well as a documentation mention and context for its use. |
XCTFail("Database should be locked") | ||
} catch { | ||
// pass | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XCTAssertThrowsError
let statement = try db.prepare("SELECT email FROM users") | ||
XCTAssert(try statement.step()) | ||
let blob = statement.row[0] as Blob | ||
XCTAssertEqual("[email protected]", String(bytes: blob.bytes, encoding: .utf8)!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not relevant to the test
ac29da7
to
5a30f29
Compare
ok - new changes pushed up with the test cleanup |
// create new db on disk in wal mode | ||
let path = temporaryFile() + ".sqlite3" | ||
let db = try Connection(.uri(path)) | ||
try db.run("PRAGMA journal_mode=WAL;") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test also passes with just
let db = try Connection(.inMemory)
looks like the exclusive lock is obtained even when WAL mode is not active
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah interesting, i wouldn't have expected a wal_checkpoint
to affect a non-wal mode db. i'll remove that line then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at this a bit more, I would change the test to not depend on any WAL specifics at all. Another way the db can end up in a locked state is when you have an open reader and then try to drop the table from the same connection:
let statement = try db.prepare("SELECT email FROM users")
_ = try statement.step()
// verify that the transaction is not closed, which prevents schema changes
XCTAssertThrowsError(try db.run("DROP TABLE users")) { error in
if case let Result.error(_, code, _) = error {
XCTAssertEqual(code, SQLITE_LOCKED)
} else {
XCTFail("unexpected error")
}
}
// reset the prepared statement, allowing the implicit transaction to close
statement.reset()
// DROP table succeeds now
try db.run("DROP TABLE users")
The db should be opened in-memory. Additionally, can you simplify the db schema? Most of the fields defined are not relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea to use the drop table - i've just pushed up a commit that uses the .inMemory
db and users
table from the setup()
method, and swaps to that drop table query
…sn't reset, update comments
For context, the sqlite docs: https://www.sqlite.org/lang_transaction.html#implicit_versus_explicit_transactions
For clients that keep and reuse prepared statements, that statement prevents transactions from fully closing (even after a
COMMIT
). This, in turn, prevents a wal checkpoint (both explicitly requested checkpoint and auto checkpoints) from succeeding. Since reset is only currently called when starting a new query or indeinit
, this means for any long-lived prepared statement will prevent a wal checkpoint, causing the wal file to grow unbounded.Instead, allowing clients to explicitly reset their prepared statements allows them to control when the transaction closes, which allows wal checkpoints to succeed.