Add option to disable database change filtering in TransactionObserver

[simolus3]

Some statements might issue writes to the databases through a layer of indirection (e.g. via a user-defined function). For these statements, GRDB is unable to detect the writes made and doesn't call TransactionObserver.observes(eventKind:), doesn't install an update listener and has no way of observing these writes.

This adds TransactionObserver.observesAllDatabaseChanges, which can be used to make transaction observers listen for all database changes. It returns false by default, but can be added to implementations to observe indirect statements as well.

Closes #1863.

Pull Request Checklist

• CONTRIBUTING: You have read https://github.com/groue/GRDB.swift/blob/master/CONTRIBUTING.md
• BRANCH: This pull request is submitted against the development branch.
• DOCUMENTATION: Inline documentation has been updated.
• DOCUMENTATION: README.md or another dedicated guide has been updated.
• TESTS: Changes are tested.
• TESTS: The make smokeTest terminal command runs without failure.

[groue]

Thank you @simolus3!

I'll review this pull request in the next few days. At first glance, it does exactly what we talked about, and it does it well 👍

Please tell me if it allows PowerSync to achieve the goals you described in #1863, and in particular if later on you can call notifyChanges(in:) with the desired effects 🙏

[simolus3]

Please tell me if it allows PowerSync to achieve the goals you described in #1863, and in particular if later on you can call notifyChanges(in:) with the desired effects 🙏

Yes, I've tested these changes with PowerSync and they seem to do exactly what we need :) My approach right now is to add a catchall observer in a writeWithoutTransaction block running statements for PowerSync, once that is complete I forward collected writes with notifyChanges(in:) in an empty transaction. This updates GRDB value observations reliably 👍

[groue]

I had a closer look, and I could spot a design flaw 🙂

Your changes closely reflect our initial intent, but I gave you a misleading direction. It won't be difficult to fix, fortunately!

---

The symptom of the design flaw is that the new test fails if the observer returns false from observes(eventsOfKind:):

 private class EverythingObserver: TransactionObserver {
     func observes(statement: Statement) -> Bool {
         return true
     }
 
     func observes(eventsOfKind eventKind: GRDB.DatabaseEventKind) -> Bool {
-        return true  // test passes
+        return false // test fails
     }
 }

This is definitely a bug. With this modification of the test, the observes(statement:) method still returns true for both DELETE FROM foo and SELECT custom_clear_function(), so we ought to be notified of deletions.

The cause is that SQLite compiles the DELETE FROM foo statement with the truncate optimization enabled. When run, the statement truncates the table instead of deleting rows one by one. Individual deletions are not notified to the update hook, and the transaction observer is blind.

And there lies the design flaw: the truncate optimization is enabled or disabled from the compile-time authorization callbacks, before we can call observes(statement:), which means before the observer can instruct GRDB to disable the truncate optimization.

---

The intention described at #1863 (comment) still holds:

[...] because we can't build an argument for observes(eventsOfKind:) before executing SELECT powersync_clear(), it is necessary to come up with a new TransactionObserver method that opts in for the events created by this statement.

It turns wrong in the next paragraph:

For example: func observes(statement: DatabaseStatement) -> Bool that returns false by default (preserving the API contract with other transaction observers that do not implement this method).

I think our only solution is to let observers opt in for all update events, without any filtering:

 protocol TransactionObserver {
-    func observes(statement: Statement) -> Bool
+    var observesAllDatabaseChanges: Bool { get }
 }

---

If you lack time, I'll update this pull request accordingly. The TransactionObserver high-level documentation needs an update, too. Probably we should start mentioning the "update hook" aka the Data Change Notification Callbacks very explicitly, without beating around the bush. We also have various paragraphs titled "Dealing with undetected changes" (1, 2, 3) that need to include "statements ran from a custom SQL function" in the possible causes for undetected changes.

[simolus3]

I think our only solution is to let observers opt in for all update events, without any filtering:

That will definitely work for PowerSync too, I've changed the PR to be based on that 👍

The TransactionObserver high-level documentation needs an update, too

I've added a small note on observesAllDatabaseChanges.

We also have various paragraphs titled "Dealing with undetected changes" (1, 2, 3) that need to include "statements ran from a custom SQL function" in the possible causes for undetected changes.

I suppose "Changes performed by SQLite statements that are not both compiled and executed through GRDB APIs" implicitly covers that (since the function executes the statement with direct SQLite APIs internally, not with GRDB APIs). But it's good to be explicit about this, and I've expanded that bullet point.

Probably we should start mentioning the "update hook" aka the Data Change Notification Callbacks very explicitly, without beating around the bush.

I'm not really sure where we'd mention that, in the section about transaction observers controlling for which events they'd be invoked?

I would be happy to expand the documentation with a bit more help from your side, but if you think it's quicker if you update this PR, then that's definitely fine with me as well :)

[groue]

Sounds great! OK I take charge of the remaining tidbits 👍 Thank you very much for your help 🙏

[groue]

Hello @simolus3, sorry for the delay. May is a lazy month in France, and a heat wave did not help 😅

I performed a last refactor, due to the misleading observesAllDatabaseChanges name. Practically speaking, transaction observers are not notified of changes that are performed in a save point that is eventually rollbacked. GRDB only notifies changes that have an actual chance of being committed to disk (see Database Changes And Transactions for more details).

By exposing a DatabaseEventObservationStrategy type, we allow for a future extra flag that ignores save points, trading observation precision for a reduced memory footprint.

The universal observer you need is now written as below:

// An observer that observes all database changes.
class UniversalObserver: TransactionObserver {
    var databaseEventObservationStrategy: DatabaseEventObservationStrategy {
        var strategy = DatabaseEventObservationStrategy.default
        // Don't filter on database event kind, so that we are
        // notified of changes performed through the SQLite C API:
        strategy.requiresDatabaseEventKind = false
        return strategy
    }

    // You still have to provide an implementation for this method,
    // but it will never be called since `requiresDatabaseEventKind`
    // is false.
    func observes(eventsOfKind eventKind: DatabaseEventKind) -> Bool {
        false // ignored
    }

    func databaseDidChange(with event: DatabaseEvent) {
        // Handle the change
    }
}

[simolus3]

Thanks for all your help with this :) I agree that DatabaseEventObservationStrategy is a nice way to make this extensible.

[groue]

Thank you for your patience. Full test suite is still running, I'll ship shortly after, probably tomorrow.

[groue]

⛵ The feature was shipped in v7.11.0 :-)