Skip to content

Catalyst fixes #52

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.

Sign up for GitHub

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

Merged
atdrendel merged 2 commits into from
Jan 30, 2024
Merged

Catalyst fixes #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
42ccab9
Fix URL(path:) on Mac Catalyst
atdrendel Jan 30, 2024
bf79410
Use immediate transactions
atdrendel Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Package.resolved
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import PackageDescription
let package = Package(
name: "SQLite",
platforms: [
.macOS(.v13), .iOS(.v16), .tvOS(.v16), .watchOS(.v9),
.macOS(.v13), .macCatalyst(.v16),
.iOS(.v16), .tvOS(.v16), .watchOS(.v9),
],
products: [
.library(
Expand All @@ -19,7 +20,7 @@ let package = Package(
),
.package(
url: "https://github.com/groue/GRDB.swift.git",
from: "6.18.0"
from: "6.24.2"
),
.package(
url: "https://github.com/shareup/precise-iso-8601-date-formatter.git",
Expand Down
21 changes: 18 additions & 3 deletions Sources/SQLite/SQLiteDatabase.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,14 @@ public final class SQLiteDatabase: DatabaseProtocol, @unchecked Sendable {
path: String,
busyTimeout: TimeInterval = 5
) throws -> SQLiteDatabase {
guard path != ":memory:", let url = URL(string: path)
else { throw SQLiteError.SQLITE_IOERR }
guard path != ":memory:" else {
throw SQLiteError.SQLITE_IOERR
}

let url: URL? = URL(string: path)
?? URL(filePath: path, directoryHint: .notDirectory)

guard let url else { throw SQLiteError.SQLITE_IOERR }

let coordinator = NSFileCoordinator(filePresenter: nil)
var fileCoordinatorError: NSError?
Expand Down Expand Up @@ -636,11 +642,20 @@ private extension SQLiteDatabase {

var config = Configuration()
config.journalMode = isInMemory ? .default : .wal
// NOTE: GRDB recommends `defaultTransactionKind` be set
// to `.immediate` in order to prevent `SQLITE_BUSY`
// errors. Using `.immediate` appears to disable
Copy link
Copy Markdown

@groue groue Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @atdrendel, thanks for trusting the GRDB doc :-) May I ask how you have discovered that immediate transactions disable automatic vacuuming? I never use PRAGMA auto_vacuum, out of sheer inexperience. It looks like immediate transactions can have a big cost in terms of disk usage for some apps?

Copy link
Copy Markdown
Contributor Author

@atdrendel atdrendel Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@groue My tests caught the problem with automatic vacuuming. When .immediate is set, the auto_vacuum pragma seems to have to effect.

I just released a nightly with this change last night. So, I can't really comment on its performance yet. I'll keep an eye on it, though.

Copy link
Copy Markdown
Contributor Author

@atdrendel atdrendel Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, my vacuum tests all failed when I enabled .immediate. I haven't looked into that yet, either, but I guess the disk size issue you mentioned could be related to that.

Copy link
Copy Markdown

@groue groue Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A search in Google and the SQLite forums does not make clear that IMMEDIATE transactions are linked to auto-vacuum.

But this post is probably related: https://sqlite.org/forum/forumpost/4cc6473ceb9038cf618282e40f235c4f35f0c08832de0bcb276f84bff6745688

The auto-vacuum only happens on a write transaction. For a read transaction, nothing has changed, and so there is nothing to auto-vacuum. Hence the write lock is already held whenever an auto-vacuum occurs.

In WAL mode, the database changes necessary to move freelist pages to the end of the database and then truncate the database are written into the WAL file. But those changes are not actually applied (and the database does not actually shrink) until the next checkpoint operation is run to move the changes in the WAL file back into the database.

Indeed I have already seen that VACUUM (auto or not) does not reduce the size of a WAL database, because… the VACUUM is registered in the WAL (I'm not discussing if it is the expected behavior or not 😅). Maybe this is what your tests have detected? Something related to the WAL mode, more than something related to immediate transactions?

Copy link
Copy Markdown
Contributor Author

@atdrendel atdrendel Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@groue I found some time to look into this issue today. The vacuuming issue was not directly caused by the IMMEDIATE transaction change. The reason for the change was my library was incorrectly using writer.write in cases when I should have used writer.writeWithoutTransaction. I addressed that problem in #53 and now all of the tests pass correctly.

Copy link
Copy Markdown

@groue groue Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your investigations 🙏

// automatic vacuuming.
//
// https://swiftpackageindex.com/groue/grdb.swift/v6.24.2/documentation/grdb/databasesharing#How-to-limit-the-SQLITEBUSY-error
config.defaultTransactionKind = isInMemory
? .deferred
: .immediate
config.busyMode = .timeout(busyTimeout)
config.observesSuspensionNotifications = true
config.maximumReaderCount = max(
ProcessInfo.processInfo.processorCount,
5
6
)

guard !isInMemory else {
Expand Down
2 changes: 1 addition & 1 deletion Sources/SQLite/SQLiteValue.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ extension [String: SQLiteValue] {
init?(row: Row?) {
guard let row else { return nil }
var sqliteRow = SQLiteRow()
row.forEach { name, value in
for (name, value) in row {
switch DatabaseValue(value: value)?.storage {
case .none, .null:
sqliteRow[name] = .null
Expand Down
18 changes: 14 additions & 4 deletions Tests/SQLiteTests/SQLiteDatabaseTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,16 @@ final class SQLiteDatabaseTests: XCTestCase {
database = nil
}

func testSQLiteVersionMeetsMinimumRequirements() throws {
try Sandbox.execute { directory in
let path = directory.appendingPathComponent("test.db").path
let db = try SQLiteDatabase(path: path)
defer { try? db.close() }
let version = try SQLiteVersion(db)
XCTAssert(version.isSupported)
}
}

func testDatabaseIsCreated() throws {
try Sandbox.execute { directory in
let path = directory.appendingPathComponent("test.db").path
Expand Down Expand Up @@ -133,7 +143,7 @@ final class SQLiteDatabaseTests: XCTestCase {
try database.execute(raw: _createTableWithBlob)

try database.inTransaction { db in
try (0 ..< 1000).forEach { index in
for index in 0 ..< 1000 {
let args: SQLiteArguments = [
"id": .integer(Int64(index)), "data": .data(_textData),
]
Expand Down Expand Up @@ -162,7 +172,7 @@ final class SQLiteDatabaseTests: XCTestCase {
try database.execute(raw: _createTableWithBlob)

try database.inTransaction { db in
try (0 ..< 1000).forEach { index in
for index in 0 ..< 1000 {
let args: SQLiteArguments = [
"id": .integer(Int64(index)), "data": .data(_textData),
]
Expand Down Expand Up @@ -191,7 +201,7 @@ final class SQLiteDatabaseTests: XCTestCase {
try database.execute(raw: _createTableWithBlob)

try database.inTransaction { db in
try (0 ..< 1000).forEach { index in
for index in 0 ..< 1000 {
let args: SQLiteArguments = [
"id": .integer(Int64(index)), "data": .data(_textData),
]
Expand Down Expand Up @@ -220,7 +230,7 @@ final class SQLiteDatabaseTests: XCTestCase {
try db1.execute(raw: _createTableWithBlob)

try db1.inTransaction { db in
try (0 ..< 1000).forEach { index in
for index in 0 ..< 1000 {
let args: SQLiteArguments = [
"id": .integer(Int64(index)), "data": .data(_textData),
]
Expand Down
6 changes: 3 additions & 3 deletions Tests/SQLiteTests/SQLitePublisherTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ final class SQLitePublisherTests: XCTestCase {
try database.execute(raw: Person.createTable)
try database.execute(raw: Pet.createTable)

try [_person1, _person2].forEach { person in
for person in [_person1, _person2] {
try database.write(Person.insert, arguments: person.asArguments)
}

try [_pet1, _pet2].forEach { pet in
for pet in [_pet1, _pet2] {
try database.write(Pet.insert, arguments: pet.asArguments)
}
}
Expand Down Expand Up @@ -104,7 +104,7 @@ final class SQLitePublisherTests: XCTestCase {
defer { try? db.close() }
try db.execute(raw: Person.createTable)

try [_person1, _person2].forEach { person in
for person in [_person1, _person2] {
try db.write(
Person.insert,
arguments: person.asArguments
Expand Down
28 changes: 14 additions & 14 deletions Tests/SQLiteTests/SQLiteRow+ExtensionsTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ final class SQLiteRowExtensionsTests: XCTestCase {
("negative", true),
]

try expected.forEach { key, expectedValue in
for (key, expectedValue) in expected {
XCTAssertEqual(expectedValue, try values.value(for: key))
XCTAssertEqual(
expectedValue,
Expand All @@ -37,7 +37,7 @@ final class SQLiteRowExtensionsTests: XCTestCase {
("nonempty", "123".data(using: .utf8)!),
]

try expected.forEach { key, expectedValue in
for (key, expectedValue) in expected {
XCTAssertEqual(expectedValue, try values.value(for: key))
XCTAssertEqual(
expectedValue,
Expand All @@ -58,7 +58,7 @@ final class SQLiteRowExtensionsTests: XCTestCase {
("2001", Date(timeIntervalSinceReferenceDate: 123)),
]

try expected.forEach { key, expectedValue in
for (key, expectedValue) in expected {
XCTAssertEqual(expectedValue, try values.value(for: key))
XCTAssertEqual(
expectedValue,
Expand All @@ -80,7 +80,7 @@ final class SQLiteRowExtensionsTests: XCTestCase {
("negative", -12_345.12345),
]

try expected.forEach { key, expectedValue in
for (key, expectedValue) in expected {
XCTAssertEqual(expectedValue, try values.value(for: key))
XCTAssertEqual(
expectedValue,
Expand All @@ -102,7 +102,7 @@ final class SQLiteRowExtensionsTests: XCTestCase {
("negative", -12_345),
]

try expected.forEach { key, expectedValue in
for (key, expectedValue) in expected {
XCTAssertEqual(expectedValue, try values.value(for: key))
XCTAssertEqual(
expectedValue,
Expand All @@ -124,7 +124,7 @@ final class SQLiteRowExtensionsTests: XCTestCase {
("negative", -12_345),
]

try expected.forEach { key, expectedValue in
for (key, expectedValue) in expected {
XCTAssertEqual(expectedValue, try values.value(for: key))
XCTAssertEqual(
expectedValue,
Expand All @@ -144,7 +144,7 @@ final class SQLiteRowExtensionsTests: XCTestCase {
("nonempty", "This is not empty"),
]

try expected.forEach { key, expectedValue in
for (key, expectedValue) in expected {
XCTAssertEqual(expectedValue, try values.value(for: key))
XCTAssertEqual(
expectedValue,
Expand All @@ -165,7 +165,7 @@ final class SQLiteRowExtensionsTests: XCTestCase {
("missing", nil),
]

try expected.forEach { key, expectedValue in
for (key, expectedValue) in expected {
XCTAssertEqual(expectedValue, try values.value(for: key))
XCTAssertEqual(
expectedValue,
Expand All @@ -186,7 +186,7 @@ final class SQLiteRowExtensionsTests: XCTestCase {
("missing", nil),
]

try expected.forEach { key, expectedValue in
for (key, expectedValue) in expected {
XCTAssertEqual(expectedValue, try values.value(for: key))
XCTAssertEqual(
expectedValue,
Expand All @@ -209,7 +209,7 @@ final class SQLiteRowExtensionsTests: XCTestCase {
("2001", Date(timeIntervalSinceReferenceDate: 123)),
]

try expected.forEach { key, expectedValue in
for (key, expectedValue) in expected {
XCTAssertEqual(expectedValue, try values.value(for: key))
XCTAssertEqual(
expectedValue,
Expand All @@ -230,7 +230,7 @@ final class SQLiteRowExtensionsTests: XCTestCase {
("missing", nil),
]

try expected.forEach { key, expectedValue in
for (key, expectedValue) in expected {
XCTAssertEqual(expectedValue, try values.value(for: key))
XCTAssertEqual(
expectedValue,
Expand All @@ -251,7 +251,7 @@ final class SQLiteRowExtensionsTests: XCTestCase {
("missing", nil),
]

try expected.forEach { key, expectedValue in
for (key, expectedValue) in expected {
XCTAssertEqual(expectedValue, try values.value(for: key))
XCTAssertEqual(
expectedValue,
Expand All @@ -272,7 +272,7 @@ final class SQLiteRowExtensionsTests: XCTestCase {
("missing", nil),
]

try expected.forEach { key, expectedValue in
for (key, expectedValue) in expected {
XCTAssertEqual(expectedValue, try values.value(for: key))
XCTAssertEqual(
expectedValue,
Expand All @@ -293,7 +293,7 @@ final class SQLiteRowExtensionsTests: XCTestCase {
("missing", nil),
]

try expected.forEach { key, expectedValue in
for (key, expectedValue) in expected {
XCTAssertEqual(expectedValue, try values.value(for: key))
XCTAssertEqual(
expectedValue,
Expand Down