[native + sqlite WAL] Read-your-writes consistency violation under stress, due to SqlCursor escaping mutual exclusion of its parent connection

[andersio]

Runtime Environment
SQLDelight version: 1.4.3
Application OS: iOS 14.x

Proposed change: #2132

TL;DR

SQLite Cursors/Statements are allowed to escape the critical section for SQLite connections. This causes intermittent read-your-writes consistency violation at least with the native driver in WAL mode.

Query listeners and after commit callbacks fail to see the written data, when the system is stressing SQLDelight.

Identified issue

In executeQuery of NativeSqlDatabase , the SqlCursor escapes the critical section provided by accessConnection().

sqldelight/drivers/native-driver/src/nativeMain/kotlin/com/squareup/sqldelight/drivers/native/NativeSqlDatabase.kt

Lines 53 to 63 in 51e0621

	final override fun executeQuery(
	identifier: Int?,
	sql: String,
	parameters: Int,
	binders: (SqlPreparedStatement.() -> Unit)?
	): SqlCursor {
	return accessConnection(false) {
	val statement = getStatement(identifier, sql)
	if (binders != null) {
	try {

This means other threads can potentially acquire and start using the query connection, while the earlier owner is still stepping the result set through the escaped SqlCursor. While concurrently accessing the SQLite connection itself isn't problematic, the escaped SqlCursor has implications in WAL mode.

WAL readers cannot move its end mark when it still has active statements. In other words, if the read/query connection is prematurely yielded from a 1st thread 1 to a 2nd thread, the 2nd thread may not see any new data written by itself through the "transction pool" write connection.

It is a race condition that violates read-your-writes consistency, in that the data are visible if and only if the 1st thread by chance closes the SqlCursor, before than the 2nd thread starts any read. Such race condition can be unintuitive to isolate and fix — imagine an application with lots of read and write queries issued from 2+ threads, that unit tests are passing with small test data sets, and that hiccups only start to emerge as it processes growing user generated data.

Attached below is a code snippet to illustrate the issue. Note that the cursor lifetime is exaggerated by using execute(): SqlCursor directly. In practice, we usually use the convenient executeAs*() methods, whose cursor lifetime beyond the critical section scales with the result set size.

val list = db.fruitQueries.all().executeAtList()
check(list.count() == 0)

// Execute out-of-transaction on `queryPool`.
val cursor = db.fruitQueries.all().execute()

launch(GlobalScope + DIspatchers.Main) {
   delay(100000)
   cursor.close()
}

db.transaction {
    // Execute as part of a transaction on `transactionPool`.
    db.fruitQueries.insert("Apple")
    db.fruitQueries.insert("Orange")

    afterCommit {
        // Execute out-of-transaction on `queryPool`.
        val list = db.fruitQueries.all().executeAtList()

        // FAIL: IllegalStateException
        //
        // Count is still zero, because the escaped cursor keeps the reader conn.
        // at an earlier DB snapshot.
        check(list.count() == 2)
    }
}

This issue has to be solved at framework level, since:

• As a user, I have no option to extend the critical section (internal to SQLDelight) to cover stepping of the cursor.
• The only user-side "solution" is to serialize all database accesses, which defeats the point of having multiple connections.

Note that this affects not only the afterCommit hook, but also any query listeners as they are both called out at the same place. Moreover, this issue is applicable to SQLite usage on any platform, since this is rooted in SQLite's API design choice.

Proposed solution

SqlCursor and any other resource should be bound to the critical section. They should be closed before the current thread releases its lock on the connection.

The signature of SqlDriver.executeQuery and Query.execute() should be changed to accept a (SqlCursor) -> R block, so that the block can be evaluated and immediately followed up with cleanup actions, right inside the accessConnection() critical section. The result of the block is passed back to the caller.

This is however an API breaking change.

Our proposed patch for the issue: #2132

interface SqlDriver {
  /**
   * Execute a SQL statement and evaluate its result set using the given block.
   *
   * @param [identifier] An opaque, unique value that can be used to implement any driver-side
   *   caching of prepared statements. If [identifier] is null, a fresh statement is required.
   * @param [sql] The SQL string to be executed.
   * @param [parameters] The number of bindable parameters [sql] contains.
   * @param [binders] A lambda which is called before execution to bind any parameters to the SQL
   *   statement.
   * @param [block] A lambda which is called with the cursor when the statement is executed
   *   successfully. The generic result of the lambda is returned to the caller, as soon as the
   *   mutual exclusion on the database connection ends. The cursor **must not escape** the block
   *   scope.
   */
  fun <R> executeQuery(
    identifier: Int?,
    sql: String,
    parameters: Int,
    binders: (SqlPreparedStatement.() -> Unit)? = null,
    block: (SqlCursor) -> R
  ): R
}

abstract class Query<T> {
  /**
   * Execute the underlying statement. The resulting cursor is passed to the given block.
   *
   * The cursor is closed automatically after the block returns.
   */
  abstract fun <R> execute(block: (SqlCursor) -> R): R
}

[kpgalligan]

Thinking through this. My understanding of the underlying Android sqlite driver would imply this may also be an issue in that context. I'd have to digest the detail to understand better, though.

When working on the driver initially, the issue was as you've pointed out. The cursor needs to hold onto the statement because the user may be scrolling through, but the driver needs to move on with life and do other things. Passing in a lambda would simplify also simplify the driver implementation. The statement cache currently removes any statement in use and re-adds them when done. We don't need to do that for write statements, because execute takes a lambda so nobody else would be able to access the statement. Passing a lambda in for queries would allow for the same. You'd need one copy of each statement in the thread's cache.

This also means sqlite3_close_v2 isn't super useful, although I don't think it would hurt to have it. I've added it to SQLiter and have run some tests, but using lambdas with queries would mean we'd be able to close everything when close is called on the connection.

[andersio]

The cursor needs to hold onto the statement because the user may be scrolling through, but the driver needs to move on with life and do other things.

It seems JDBC and Android has a higher level Cursor that does automatic windowing. Looks like it involves modifying the SQL statement with LIMIT ? OFFSET ? behind the curtains, and does lazy fetching in batches as one advances the cursor.

As for whether the JDBC/Android drivers are prone to the same issue, it depends on whether their Cursor implementation starts a read transaction that lasts as long as the cursor itself:

• If they do, the issue is applicable, since the read transaction will keep the connection from seeing new data in WAL mode, or block any writers in rollback journal mode.

• If they don't, the issue does not apply. But now depending on whether auto-commit mode is on (~= wrapping each SELECT statement in an implicit read transaction), it can be an ACID violation.

  More specifically, pages fetched over time are no longer guaranteed to be consistent with each other, because there is no isolation protection by a read transaction. Say, a concurrent writer could have inserted new rows, that cause the classic array index invalidation problem (if you imagine the abstract sorted result set as an array).

  One funny trivia is that some JDBC drivers ignore windowed cursor settings by default, and always execute the query in full, unless auto-commit mode is turned off. Both MySQL and PostgreSQL, AFAIK.

In any case, this is really an opinioniated question, on whether there is a value in exposing this JDBC/Android magic cursor behavior. The android-paging extension seems to indicate a NO, since it does require users to write such query explicitly in a .sq file, and pass the generated Query to it.

[kpgalligan]

Thought about this some more. Commented on the lambda proposal, which I like conceptually. As an alternative, what if the connection remained aligned to the thread until the cursor is returned? That would prevent new queries on that connection until the previous one was closed. Still need to rethink the connection management somewhat, but that's been in progress anyway (at least conceptually).

[kpgalligan]

Yeah, I'm familiar with the Android driver's magic window. I have no experience with the JDBC driver.

pages fetched over time are no longer guaranteed to be consistent with each other, because there is no isolation protection by a read transaction

Was thinking about that. Don't know how that and auto-commit would play out. I'd have to refresh how that driver works. Not implementing something like that would obviously be ideal.

[andersio]

As an alternative, what if the connection remained aligned to the thread until the cursor is returned?

That would help, but a reader pool with a default max > 1 across all drivers might become essential. That or the reader pool needs to impose a soft cap* on reader concurrency instead to guarantee forward progress, i.e., not to be blocked by threads having long-running cursors.

* e.g., serve one-off throwaway reader connections when the pool is exhausted of ready-to-use connections.

[kpgalligan]

I would agree, but assuming the dev isn't holding onto the cursor on purpose and just looping through, wouldn't the lambda have the same issue with a reader pool?

[kpgalligan]

I mean, the reader pool also needs some thinking, but holding onto the connection or passing the lambda would have the same problem (I think, unless I'm missing something).

[andersio]

I would agree, but assuming the dev isn't holding onto the cursor on purpose and just looping through, wouldn't the lambda have the same issue with a reader pool?

Indeed yes, #2132 can still block progress if the stepping takes long, just that the cursor is guaranteed to close, before the connection is returned to the pool for reuse.

So "guaranteeing forward progress" is more in the context of users that will indeed hold onto a cursor.

---

Having thought again about aligning threads to a connection until cursor closure, I now think that won't be able to address the read-your-writes consistency violation. It seems in Android, cursors can be taken across thread, or even on the main thread (?).

Most importantly, when one works in a thread pool or non-blocking environment (e.g., coroutines), work that holds onto a cursor might be interleaved with other work reusing the same thread. So we'd end up where we are at today.

[kpgalligan]

It seems in Android, cursors can be taken across thread, or even on the main thread (?).

Yeah, that's an issue. Again, I like the lambda idea, but thinking through alternatives. We'd have to rely on the user not dragging the cursor across threads, and always closing it appropriately, which is problematic.

Most importantly, when one works in a thread pool or non-blocking environment (e.g., coroutines), work that holds onto a cursor might be interleaved with other work reusing the same thread.

This is definitely a pending issue, and it will need to be addressed, but native coroutines aren't thread pooled (today), so we've kicked that a bit into the future to resolve.

Detailed post from an Android engineer discussing the problem and resolution in the driver. The Android driver also used to align transactions to threads, but that doesn't work with coroutines thread pooling.

https://medium.com/androiddevelopers/threading-models-in-coroutines-and-android-sqlite-api-6cab11f7eb90

[andersio]

A possible approach to keep long-running cursor support on top of #2132 could be that:

1. SqlDriver supports creating an explicit connection that is outside the pool; and

   e.g. val reader = driver.createExplicitReader()

2. Query can be executed with a long-running cursor as a result, but only when an explicit connection is provided.

   e.g. val cursor = db.articleQueries.getAll().executeIn(reader)

In other words, keep the long-running cursors away from the connection pools.

[kpgalligan]

It would be good to understand the use cases for the cursor. If it's not to pass around, but to avoid creating a big list of data objects, the lambda route makes more sense. In any case, see if anybody else weighs in.