Thinking about context cancellation

[michaellenaghan]

You mentioned that vacuum won’t work in the presence of an active statement (here).

That’s not the only example.

Collations can’t be modified or deleted (here).

Functions can’t be modified or deleted (here).

After a SQLITE3_SCHEMA error sqlite3_changes() is set to zero if there's more than one active statements (here).

This pager snapshot check only happens if there are no active statements.

There might be others, but that’s already too many!

An always-active statement has visible side-effects.

---

Given that Prepare, Step and Exec all re-apply interrupts it’s not clear to me that you need an always active statement anyway. I'm not sure it's worth puzzling that out, though, because I think there’s another issue: contexts have dynamic extent while interrupts affect all active statements in a connection.

Consider this calling pattern:

Query
    Next
        QueryContext(ctxInner)
            Next

Or this one:

QueryContext(ctxOuter)
    Next
        QueryContext(ctxInner)
            Next

In both cases cancelling ctxInner will interrupt both the inner and outer queries.

I'm not sure that’s what a programmer reading the code would expect?

---

If you don’t think the second issue is a real issue, I see two options.

Both abandon the use of an always-active statement.

1. The rest of the code stays as it is. SQLite will be re-interrupted at all of the main entry points. It might take a little longer for everything to stop, but stop it will.

2. Have checkInterrupt return context.Err() (after interrupting SQLite), and have the routines that call it early exit in the normal Go way, returning that error when it isn't nil — i.e. without calling in to SQLite. There are pros and cons to that approach.

If you think the second issue is a real issue, I see two options.

Both abandon the use of an always-active statement and interrupt.

3. Like (2) early exit, returning context.Err() if it isn't nil — but without the intervening call to interrupt.

4. Let the progress callback (and busy handler) eventually “interrupt” long running queries if the context is cancelled, and don't intervene anywhere else.

I think both of those options will respect the dynamic extent of contexts; only the queries that correspond to a cancelled context will (eventually) be “interrupted.”

Personally I think the second issue is a real issue based on my understanding of your underlying goal: predictability. By which I mean: the ability to build a correct mental model, and anticipate its implications.

---

If you go with (3) or (4) I’d suggest renaming (deprecating) SetInterrupt() in favour of SetContext(); renaming (deprecating) GetInterrupt() in favour of Context(); and renaming interrupt to ctx. That would just make things easier for Go programmers to understand.

---

As I said in the other discussion, this isn't a performance issue; I don't think these changes would have a major impact on performance.

[ncruces]

I made some formatting tweaks to your post, hope you don't mind.

The code had already changed since the previous discussion, which fixed some of your concerns.

The statement is now only made active once the context cancels:

go-sqlite3/conn.go

Lines 371 to 380 in 9e4258b

	func (c *Conn) checkInterrupt() {
	if c.interrupt.Err() == nil {
	return
	}
	if !c.stepped {
	c.call("sqlite3_step", stk_t(c.pending))
	c.stepped = true
	}
	c.call("sqlite3_interrupt", stk_t(c.handle))
	}

And then it's reset as soon as a new, not-yet-cancelled context is set:

go-sqlite3/conn.go

Lines 343 to 369 in 9e4258b

	func (c *Conn) SetInterrupt(ctx context.Context) (old context.Context) {
	old = c.interrupt
	c.interrupt = ctx
	if ctx == old {
	return old
	}
	// An active SQL statement prevents SQLite from ignoring an interrupt
	// that comes before any other statements are started.
	if c.pending == 0 {
	defer c.arena.mark()()
	stmtPtr := c.arena.new(ptrlen)
	textPtr := c.arena.string(`SELECT 0 UNION ALL SELECT 0`)
	c.call("sqlite3_prepare_v3", stk_t(c.handle), stk_t(textPtr), math.MaxUint64,
	stk_t(PREPARE_PERSISTENT), stk_t(stmtPtr), 0)
	c.pending = util.Read32[ptr_t](c.mod, stmtPtr)
	}
	if c.stepped && ctx.Err() == nil {
	c.call("sqlite3_reset", stk_t(c.pending))
	c.stepped = false
	} else {
	c.checkInterrupt()
	}
	return old
	}

So the statement is active only while the interruption lasts.

---

You do make a good point that cancelling ctxInner cancels both queries. Your option (4) is IMO impossible to reliably test. I implemented option (3) in #251. I'm not sure how reliable this is, but lets find out.

I disagree with renaming stuff. Contexts are about more than cancellation, and I don't want to give anyone the impression they can use them for anything else. Particularly, because I reserve the right to change the interrupt context momentarily (as I do/did in various places).

[ncruces]

@michaellenaghan, I merged #251. Leaving this open for a while for further discussion.

[michaellenaghan]

I'll think about it a bit more, but in a first pass it looks quite good. Much simpler.

Here are two things I'm trying to work through.

There's a potential race of sorts.

Scenario 1: User calls Step(), context is cancelled, progress callback is called, progress callback returns true, Step() eventually returns INTERRUPT.

Scenario 2: Context is cancelled, user calls Step(), Step() immediately returns INTERRUPT.

In both scenarios the user gets the same error code, but one was from SQLite and one was from ncruces. In the first scenario there may have been some automatic cleanup (e.g., here), in the second there wasn't.

Probably doesn't matter, probably OK, query should eventually get rolled back, statement should eventually get reset, but if there is a difference it would be one of those "sometimes something weird happens" things, all but impossible to understand.

Your convenience Exec() method calls Step() in a for loop, and then returns the value of Reset(). The short-circuit INTERRUPT that Step() returned will be lost.

[ncruces]

In backup.go: Should Backup() be interruptible?

I'm don't think so. It acquires locks, and may return BUSY due to cancellation, but doesn't use the VDBE and doesn't return INTERRUPT.

In conn.go: busyCallback is the one and only place that checks to see if c.interrupt is nil. Maybe it can be called during openDB before newConn has set the value? Not sure.

That's historic. It should never be nil. The new SetInterrupt doesn't check, maybe it should.

In txn.go: Begin() calls c.exec() and panic()s on err, while BeginConcurrent(), BeginImmediate(), and BeginExclusive() call c.Exec() and return the error. So Begin() can't be interrupted (as the comment says) while the others can be. Maybe...? Not sure. (Savepoint() gets the same treatment as Begin().)

Begin and Savepoint shouldn't ever return an error. Ever. Hence they panic.

In txn.go: In multiple places there's an assumption that if GetAutocommit() returns false there's nothing to Commit() or Rollback(). I'm trying to figure out whether or not the underlying assumption there is now broken.

It's not, I don't think.

Imagine there's no explicit BEGIN statement, so we're in autocommit mode. Imagine a DELETE query that's using a recursive CTE to delete a tree of rows — which means it will have to be Step()ed multiple times. Imagine that a Step() call returns sqlite3.INTERRUPT from the ncruces layer — i.e., SQLite didn't do an auto-rollback. Wouldn't GetAutocommit() return true while there's a transaction in progress that needs to be committed or rolled back? Should you be using TxnState() instead? Not sure.

I'm pretty sure this is not the case. Single statement transactions don't change the auto-commit mode.

In driver/driver.go: Commit() calls Rollback() on error, but only if !c.Conn.GetAutocommit().

It only rolls back if there's anything to rollback. Otherwise, rollback would return an error, which we'd have to ignore.

This should all be OK if you follow the rules and don't use the same connection across goroutines.

https://sqlite.org/lang_transaction.html
https://sqlite.org/c3ref/get_autocommit.html

[michaellenaghan]

Begin and Savepoint shouldn't ever return an error. Ever. Hence they panic.

Actually, I was making a different point: after a context is cancelled you can make forward progress with Begin(), but not with BeginConcurrent(), etc. It's not a big deal, but I wasn't sure why they'd be different.

(Btw, I think BEGIN returns an error if you nest transactions? Not sure that should result in a panic?)

In multiple places there's an assumption that if GetAutocommit() returns false there's nothing to Commit() or Rollback().

Just pointing out that I edited the false to true, but apparently a little too late!

I'm pretty sure this is not the case. Single statement transactions don't change the auto-commit mode.

Messages crossed, so you didn't see the test results, but your phrasing makes me wonder if we're talking about the same thing.

I agree that single statement transactions don't change the auto-commit mode. But that was kind of my point. End() and Release() and Commit() all assume that if GetAutocommit() returns true there's nothing to commit or roll back. As the test results show, that isn't correct; you can have GetAutocommit() == true while TxnState() != 0 — meaning there is something to commit or roll back.

[michaellenaghan]

It only rolls back if there's anything to rollback. Otherwise, rollback would return an error, which we'd have to ignore.

I confirmed that when GetAutocommit() == true and TxnState() == 2 Exec()ing ROLLBACK fails with an error.

Reset()ing the statement, however, brings TxnState() back to 0.

Hmmm.

So I think the driver code is OK — the statement is reset when the rows are closed — and "direct" code would depend on how, for example, you implemented persisted prepared statements. But that's a user thing, not an implementer thing.

[ncruces]

The point is, I don't think you can COMMIT or ROLLBACK the pseudo-transaction that gets created to execute a single statement in auto-commit mode.

[ncruces]

(Btw, I think BEGIN returns an error if you nest transactions? Not sure that should result in a panic?)

Well, I'll be upfront and say I didn't think of that (and that I initially copied the design from crawshaw/sqlite).

But, that's a programmer error (in fact, most Prepare errors should be programmer errors) and panic is fine for programmer errors IMO. This is not strong enough to change my mind and the API. You can always not use Txn: it's a helper built around the convinience of defer txn.End(&err).

I'll document the behavior, though.

[michaellenaghan]

I extended the test to add a call to cancel(). The result is the same, it's just a more realistic scenario.

func TestAutocommit(t *testing.T) {
	db := newPopulatedDB(t, *defaultMaxReadConnections, *defaultMaxWriteConnections, *defaultPosts, *defaultPostParagraphs, *defaultComments, *defaultCommentParagraphs)
	defer db.Close()

	conn, err := db.writePool.Get(t.Context())
	noErr(t, err)
	defer db.writePool.Put(conn)

	ctx, cancel := context.WithCancel(t.Context())
	defer cancel()

	_ctx := conn.SetInterrupt(ctx)
	defer conn.SetInterrupt(_ctx)

	s, _, err := conn.Prepare(`
		DELETE FROM comments
		WHERE id <= 10
		RETURNING id;`)
	noErr(t, err)
	defer s.Close()

	t.Log("Before Step")
	t.Log("Autocommit", conn.GetAutocommit())
	t.Log("TxnState", conn.TxnState(""))

	for s.Step() {
		t.Log("During Step")
		t.Log("Autocommit", conn.GetAutocommit())
		t.Log("TxnState", conn.TxnState(""))

		cancel()
	}

	t.Log("After Step")
	t.Log("Autocommit", conn.GetAutocommit())
	t.Log("TxnState", conn.TxnState(""))
}

=== RUN   TestAutocommit
    go-sqlite-bench_test.go:47: Before Step
    go-sqlite-bench_test.go:48: Autocommit true
    go-sqlite-bench_test.go:49: TxnState 0

    go-sqlite-bench_test.go:52: During Step
    go-sqlite-bench_test.go:53: Autocommit true
    go-sqlite-bench_test.go:54: TxnState 2

    go-sqlite-bench_test.go:59: After Step
    go-sqlite-bench_test.go:60: Autocommit true
    go-sqlite-bench_test.go:61: TxnState 2