sqliteutil.Save does not rollback properly after SQLITE_INTERRUPT

[fasterthanlime]

This is a sister issue to #20

When a long-running query gets interrupted (because the context is cancelled), then we reach this code:

https://github.com/crawshaw/sqlite/blob/master/sqliteutil/savepoint.go#L76-L93

The problem here is that none of those Exec statements will actually get executed if we've been interrupted.

They'll all stop either in conn.prepare() (if the statement isn't cached yet), or in conn.Step() - the point is none of that rollback SQL will run.

This is where my ForceReset() approach from #20 doesn't really work. We also need to ForceExec(), etc.

Perhaps a better solution would be to have a facility for temporarily disabling interrupted checks.

[crawshaw]

You're absolutely right that ROLLBACK should run after interrupt. Tricky! This probably needs more API surface in the sqlite package. Let me see what I can come up with.

[fasterthanlime]

You're absolutely right that ROLLBACK should run after interrupt. Tricky! This probably needs more API surface in the sqlite package. Let me see what I can come up with.

One idea is to

releaseFn := func {
  if needRollback {
    defer conn.BypassInterrupts()()
    Exec("... ROLLBACK ...")
  }
}

Where BypassInterrupts would look like:

func (conn *Conn) BypassInterrupts() func {
  conn.bypassInterrupts = true
  return conn.enableInterrupts
}

func (conn *Conn) enableInterrupts() {
  conn.bypassInterrupts = false
}

(bypassInterrupts would be checked in conn.interrupted)

I don't have a precise idea of how costly defer are, but I like this pattern a lot (discovered it in this library!), because it makes it pretty much impossible to accidentally end up with conn.bypassInterrupts = false

[crawshaw]

I think an IsInterrupted method on conn is enough. Then must-run code like rollback can:

interrupted := conn.IsInterrupted()
if interrupted {
    conn.SetInterrupt(nil)
}
// execute ROLLBACK
if interrupted {
    ch := make(chan struct{})
    close(ch)
    conn.SetInterrupt(ch)
}

[fasterthanlime]

LGTM - it sorta bothers me that doneCh's value will be changed, but I guess if we've been interrupted, it doesn't matter.

(Also maybe cache the closed chan in a package-level variable if make()-ing it isn't zero-cost and that doesn't have terrible consequences down the line?)

[crawshaw]

Turns out an IsInterrupted method would have been racy. So I made SetInterrupt return the old doneCh so the releaseFn can swap it out and back. Thanks!

[AdamSLevy]

This issue was opened under the faulty assumption that SAVEPOINTs always need to be rolled back and released after a SQLITE_INTERRUPT occurs. But there are times when the SAVEPOINTs are automatically rolled back.

This introduced a different subtle bug described in #73