Faster alternative to pool.makeSnapshot which updates existing DatabaseSnapshot

[michaelkirk-signal]

What did you do?

I'm using DatabaseSnapshot as a stable data source for my UI and hitting some performance bottlenecks when repeatedly rebuilding the DatabaseSnapshot.

From what I've read, the documentation recommends building a new DatabaseSnapshot and discarding the old one when you need to update it, but this can be quite expensive.

In my app, database writes occur mostly in the background. After a write, I need to update my UI's DatabaseSnapshot, then notify any stale views to re-render with the latest snapshot. This notification occurs as part of my UIDatabaseDelegate protocol in the following example:

protocol UIDatabaseDelegate {
    func uiDatabaseWillUpdate(_ uiDatabase: UIDatabase)
    func uiDatabaseDidUpdate(_ uiDatabase: UIDatabase)
}

class UIDatabase {
    let pool: DatabasePool
    var latestSnapshot: DatabaseSnapshot
    var delegates: [Weak<UIDatabaseDelegate>] = []

    init(pool: DatabasePool) {
        self.pool = pool
        self.latestSnapshot = try! pool.makeSnapshot()
    }
}

extension UIDatabase: TransactionObserver {
    func databaseDidCommit(_ db: Database) {
        // This is expensive!
        let newSnapshot =  try! pool.makeSnapshot()

        DispatchQueue.main.async {
            delegates.forEach { $0.value?.uiDatabaseWillUpdate(self) }

            self.latestSnapshot = newSnapshot

            // delegates re-render stale views
            delegates.forEach { $0.value?.uiDatabaseDidUpdate(self) }
        }
    }

    // ... snipped for brevity
}

In practice the UIDatabaseDelegate is usually almost equivalent to the set of ViewControllers on the navigation stack (typically 2-4 items).

Since snapshot isolation is transaction isolation, rather than building a new snapshot, it seems like we could do something cheaper, along the lines of:

extension UIDatabase: TransactionObserver {
    public func databaseDidCommit(_ db: Database) {
        DispatchQueue.main.async {
            delegates.forEach { $0.value?.uiDatabaseWillUpdate(self) }

            // the following is an incomplete implementation, do not use
            databaseSnapshot.read { db in
                // [1] end the old transaction from the old db state
                try! db.commit() 

                // [2] open a new transaction from the current db state
                try! db.beginSnapshotIsolation() // <- this is an internal method, so this won't actually compile
            }

            // delegates re-render stale views
            delegates.forEach { $0.value?.uiDatabaseDidUpdate(self) }
        }
    }
    // ... snipped for brevity
}

Intuitively, and in my profiling, starting a new transaction is much faster than creating a new snapshot (which I believe opens a new database connection). However, it introduces some immediate questions:

1. concurrency - can we be sure the new transaction at [2] reflects the state of the db that was just committed? I think so, because didCommit happens within the context of SchedulingWatchdog.preconditionValidQueue(database) so no interstitial could have occurred.

2. beginSnapshotIsolation is currently an internal method. Could it be public? Or could we encapsulate something like the above in a public try databaseSnapshot.updateToLatest() method? Is that something you'd be interested in supporting?

3. would this have an impact on the db's ability to efficiently checkpoint?

I'm also curious if you think this is a reasonable approach generally, or if this is not how you envisioned DatabaseSnapshot being used.

I'm aware of the more targeted Observer flavors GRDB offers, e.g. each view has a view model based on the Records it uses. The same query(ies) that build the view models can be fed to an observer, etc. And although I might like to move more in that direction in the future, it doesn't mesh well with the current app architecture.

Environment

GRDB flavor(s): GRDB+SQLCipher
GRDB version: 4.3.0
Installation method: CocoaPods
Xcode version: 10.3
Swift version: 5.0.1
Platform(s) running GRDB: iOS
macOS version running Xcode: 10.14.6

[groue]

Hello @michaelkirk-signal,

I'm aware of the more targeted Observer flavors

I won't discuss your architecture :-) I'm not a snapshot user myself, so issues like this one are very useful.

Thanks for the detailed context: it is easy to answer your questions.

concurrency - can we be sure the new transaction at [2] reflects the state of the db that was just committed? I think so, because didCommit happens within the context of SchedulingWatchdog.preconditionValidQueue(database) so no interstitial could have occurred.

Yes, definitely.

You use the synchronous snapshot read in the databaseDidCommit callback which is serialized with all writes, so the snaphot is refreshed before any new writes can happen.

A transaction was just committed, so you are perfectly sure of the db content.

And you use beginSnapshotIsolation so you make sure the snapshot will refer to the current database state, the exact one right after that transaction has been committed.

If you had started a plain transaction instead (beginTransaction), the database state referred by the snapshot would be postponed until the first read, so future writes would have their word. Would it be bad for your app? It is a tough concurrency question.

in your case, maybe beginTransaction would be enough, because I'm not sure you actually care about the actual db state notified by uiDatabaseDidUpdate. The difference with beginSnapshotIsolation would kick in when several fast writes happen: the main thread would have to process more distinct db states than it may really care about.

I'm not sure the stronger guarantee of beginSnapshotIsolation does really harm, so let's continue from your idea. But I'd be happy if you could tell if the weaker version would be enough for your app.

beginSnapshotIsolation is currently an internal method. Could it be public? Or could we encapsulate something like the above in a public try databaseSnapshot.updateToLatest() method? Is that something you'd be interested in supporting?

I agree that creating a new connection for each snapshot is far from ideal. Refreshing an existing one is a natural need.

It's time we had a serious look at the recently introduced SQLite snapshots, see what they allow, and how they could influence our snapshotting API.

Today I recommend that you copy the implementation of beginSnapshotIsolation in your app (if you really need it).

would this have an impact on the db's ability to efficiently checkpoint?

I lack experience here. I know that open transactions do alter checkpointing, may return errors depending on the checkpoint mode, and that the WAL can grow really big if you keep long running transactions.

[groue]

Today I recommend that you copy the implementation of beginSnapshotIsolation in your app (if you really need it).

It uses internal APIs, so it can't be copied right away. What's important is that you read something after the transaction has begun:

try db.beginTransaction(.deferred)
// Read anything
_ = try Row.fetchCursor(db, sql: "SELECT rootpage FROM sqlite_master LIMIT 1").next()

[michaelkirk-signal]

I'm not sure the stronger guarantee of beginSnapshotIsolation does really harm, so let's continue from your idea. But I'd be happy if you could tell if the weaker version would be enough for your app.

In our case, we accumulate details about the specific changes of a commit, and then need to communicate the aggregated details in lock step with the ui database snapshot being updated.

Powering a collection view is a good example, because it needs to know not only that the db did change, but also needs some specifics of how it changed so that we can do an efficient update.

Let me flesh out my example a little more:

extension UIDatabase: TransactionObserver {
    var changeDetails: DatabaseChangeDetails  = .empty 
    public func databaseDidChange(with event: DatabaseEvent) {
        // [0] accumulate changes of interest as they happen during the transaction
        // e.g. (updated: [foo], inserted: [bar, baz], deleted: [qux])
        changeDetails.addChangeDetailsForEvent(event)
    }

    public func databaseDidCommit(_ db: Database) {
        DispatchQueue.main.async {
            delegates.forEach { $0.value?.uiDatabaseWillUpdate(self) }

            let committedChangeDetails = self.changeDetails
            self.changeDetails = .empty

            // the following is an incomplete implementation, do not use
            databaseSnapshot.read { db in
                // [1] end the old transaction from the old db state
                try! db.commit() 

                // [2] open a new transaction from the current db state
                try! db.beginSnapshotIsolation() // <- this is an internal method, so this won't actually compile
            }

            // [3] using commitedChangeDetails, the delegate infers precisely *which* views were affected
            // and uses `self.databaseSnapshot` to fetch what's needed to redraw those affected views.
            delegates.forEach { $0.value?.uiDatabaseDidUpdate(self, changeDetails: commitedChangeDetails) }
        }
    }

    public func databaseDidRollback(_ db: Database) {
        self.changeDetails = .empty
    }
}

But I'd be happy if you could tell if the weaker version would be enough for your app.

So I don't think the weaker version would work for us. It would be problematic if at [3] the contents of UIDatabase.databaseSnapshot did not correspond with the contents of changeDetails.

Today I recommend that you copy the implementation of beginSnapshotIsolation in your app (if you really need it).

Thanks!

[groue]

So I don't think the weaker version would work for us.

All right Michael! This is an important "data point".

I also remember #602: changing the passphrase of an encrypted database creates some difficult concurrency-related work for the applications that use snapshots.

I'm still trying to figure out where this will drive us.

A very quick glance at SQLite snapshots seems to open new possibilities. Basically, snapshotting could become independent from particular transactions in particular connections, as currently implemented in GRDB. Several SQLite connections could share the same SQLite snapshot. This could make GRDB snapshots able to perform concurrent reads. We could also have GRDB snapshots use their parent pool's reader connections, and this would both solve the concurrency difficulties created with #602, and better honor the maximumReaderCount configuration.

It will take some time before a plan emerges. Meanwhile, I hope you will be able to refresh your snapshots as described above!

[groue]

[SQLite snapshots] could make GRDB snapshots able to perform concurrent reads

It looks like it is true.

We could also have GRDB snapshots use their parent pool's reader connections

It looks like it is wrong. Once a SQLite connection has been puts in the rails of a snapshot with sqlite3_snapshot_open, it can still jump to another snapshot (as far as I understand), but it can't jump back to regular operations. So we can't share the read-only connections of a database pool between regular reads and snapshot reads. Confirmation requested on the SQLite mailing list.

Sigh

[groue]

We could also have GRDB snapshots use their parent pool's reader connections

It looks like it is wrong [...] Confirmation requested on the SQLite mailing list.

Cool, it looks I was wrong thinking I was wrong

[groue]

SQLite snapshots can be invalidated by WAL checkpoints. This is not very cool for us, because any write can trigger an automatic checkpoint. Refactoring GRDB snapshots on top of SQLite snapshots would require disabling automatic checkpoint. I asked for a confirmation.

[groue]

For information, all experiments with SQLite snapshots happen in this branch: https://github.com/groue/GRDB.swift/tree/dev/SharedDatabaseSnapshot

To build and test, run make SQLiteCustom in the terminal, and play with the GRDBCustomSQLiteOSX scheme of GRDB.xcworkspace

[groue]

@michaelkirk-signal, during my exploration of snapshots, I happen to face some questions if we allow snapshots to be refreshed. For example, since snapshots conform to DatabaseReader, one is allowed to start a database observation from them. It's easy when snapshots are immutable (there is no change to observe in an immutable snapshot). It gets less easy if snapshots can be refreshed.

FYI, I'm planning to keep snapshots totally immutable, without any refresh method, for the sake of simplicity. I just hope that refactoring GRDB snapshots on top of SQLite snapshots, and reusing the pool's reader connections will lift the performance issues you have profiled:

in my profiling, starting a new transaction is much faster than creating a new snapshot

I admit I'm surprised that this difference makes a difference in your app. Would you be able to elaborate a little more?

[groue]

Draft PR: #625

[michaelkirk-signal]

I admit I'm surprised that this difference makes a difference in your app. Would you be able to elaborate a little more?

I've put together an example project for you, if you'd like to see:
https://github.com/michaelkirk-signal/example-grdb-database-snapshot

It also includes a couple of screenshots from Instruments.

That example is vastly simplified, but similar in spirit to how DB changes make their way to our views.

You can toggle the commented out section in Example.swift to compare the performance behavior of rebuilding snapshots vs. fast-forwarding them.

[groue]

Thank you very much @michaelkirk-signal ! I'll be able to check how #625 behaves as well 👍

[groue]

@michaelkirk-signal, here are the results of the experiments with your sample app, on my machine.

First screenshot is with regular snapshots that are re-created (your option 1):

Second screenshot is with regular snapshots that are re-used (your option 2):

Third screenshot is with the new "shared snapshots" (will have to find a better name) introduced in #625:

🔥 It looks like it's the way to go 😄

I had plenty of discussions on the SQLite mailing list in order to make sure GRDB makes a correct use of SQLite snapshots. I'm now confident.

The unexpected difficulty is that the System SQLite, which ships with the ability to use SQLite snapshots, misses the required declaration in <sqlite3.h> 🤦‍♂. In order to test your SPM app I had to manually modify GRDB (commenting out all #if SQLITE_ENABLE_SNAPSHOT), and the groue/CSQLite package (so that it adds the missing declarations in its shim.h).

What will be hard is the packaging 😒

---

One final note. Your sample code recreates snapshots from the main queue. This means that you do not have a snapshot in the exact state left by the transaction that has just ended: many concurrent transactions may have been completed before the main queue has the opportunity to grab its snapshot. This code, in practice, runs in the "weak" mode we have described in #619 (comment), despite your claim that your app needs to process each individual changes.

The strong version is the following:

func databaseDidCommit(_ db: Database) {
    // Take the snapshot before any other write can modify the db
    let snapshot = try! self.storage.pool.makeSnapshot()
    DispatchQueue.main.async {
        self.latestSnapshot = snapshot
        self.updateUI()
    }
}

With the new shared snapshots, the strong version will read as below (and it will be fast unless I'm mistaken):

func databaseDidCommit(_ db: Database) {
    // Take the snapshot before any other write can modify the db
    let snapshot = try! self.storage.pool.makeSharedSnapshot()
    DispatchQueue.main.async {
        self.latestSnapshot = snapshot
        self.updateUI()
    }
}

[groue]

I also want to stress out that the "shared snapshots" introduced in #625 and WAL checkpointing are incompatible. #625 prevents automatic and explicit checkpoints as long as there are living shared snapshots, so that snapshots are reliable.

There is no free lunch.

With shared snapshots:

• 👍 You gain fast snapshot creation.

• 👍 You spare memory, and you gain concurrent snapshot reads (shared snapshots use the same pool of connections as regular database pool reads)

• 👎 You lose checkpointing and the WAL can grow indefinitely (bad) unless you make sure you destroy all your shared snapshots, from time to time, so that automatic or explicit checkpoints can complete.

• 👎 You MUST absolutely prevent any external SQLite connection from writing in the database (absolutely all writes must happen through the database pool that has created the snapshots) or checkpoints will run, which means that snapshots will get invalidated and will throw an error on each attempt to read from them.

I think this information is worth considered very closely :-)

[michaelkirk-signal]

One final note. Your sample code recreates snapshots from the main queue. This means that you do not have a snapshot in the exact state left by the transaction that has just ended

Ah yes, that was an error I introduced when writing the example app. 😓We were in fact doing something like your strong example, which creates the snapshot synchronously in didCommit, and the main queue is only used for assignment of the snapshot.

You lose checkpointing and the WAL can grow indefinitely (bad) unless you make sure you destroy all your shared snapshots, from time to time, so that automatic or explicit checkpoints can complete.

Obviously we'd need to checkpoint eventually. This strategy seems doable.

If you have existing shared snapshots, is it that checkpointing will be unproductive, or that it will inadvertently update the db state seen by shared snapshot?

You MUST absolutely prevent any external SQLite connection from writing in the database (absolutely all writes must happen through the database pool that has created the snapshots) or checkpoints will run, which means that snapshots will get invalidated and will throw an error on each attempt to read from them.

I'm not familiar with the implementation of shared snapshots, but it sounds incompatible with having a share extension. Is that true? That would be a deal breaker unfortunately.

[groue]

Ah yes, that was an error I introduced when writing the example app.

Don't worry - I just had to spot it just in case :-)

Obviously we'd need to checkpoint eventually. This strategy seems doable.

If you have existing shared snapshots, is it that checkpointing will be unproductive, or that it will inadvertently update the db state seen by shared snapshot?

When snapshots are protected, automatic checkpoint is disabled, and explicit checkpoints fail with an error.

Recent commits in #625 has made such protection disabled by default. "Historical snapshots" (a new name that is closer to original SQLite Documentation) are fragile in the intention of the SQLite authors. They are fragile because any checkpoint invalidates them - and it's impossible to protect them absolutely. I think this is why they are so fast: there is no need to create any lock on disk in order to create one - but external connections are unaware of their existence.

Protection for a given database pool can be turned on explicitly:

var config = Configuration()
config.fragileHistoricalSnapshots = false
let dbPool = try DatabasePool(..., configuration: config)

But, as you have understood:

it sounds incompatible with having a share extension

If the extension wants to write, then yes it can invalidate snapshots: existing snapshots will throw errors.

In case you'd have any doubt, this fragility does not apply to "regular" snapshots returned by DatabasePool.makeSnapshot() which are made of one connection and one transaction. Those are super robust even when other connections want to write.

[michaelkirk-signal]

As always, thank you for the clear explanation @groue!

[groue]

We have discovered a new area of SQLite :-)

So it looks like you will stick with regular snapshots. Remains the question of making them mutable with a public refresh method.

I'm still slightly reluctant to do it. With the transaction refresh, you're living on the edge by using private implementation details. It may break one day, but it won't break soon :-) I think it's not bad, you and I have time to gather more data and evidence: we're all learning.

If you think we're done, you may close this issue :-)