sqlite icon indicating copy to clipboard operation
sqlite copied to clipboard

Handle SQLITE_BUSY gracefully

Open FiloSottile opened this issue 6 years ago • 10 comments

Now that the shared cache is disabled and WAL-level locking is used directly, I am seeing a lot of errors like sqlite.Exec: Stmt.Step: SQLITE_BUSY: database is locked surface to the application.

They should be transparently handled by sqlite3_busy_handler just like SQLITE_LOCKED was handled with sqlite3_unlock_notify.

FiloSottile avatar Dec 17 '18 02:12 FiloSottile

Interesting. Any idea how long you're holding transactions for?

I've considered doing this, but connections are already using sqlite3_busy_timeout with a 10 second timeout, and in everything I'm doing that's much longer than I ever actually hold a TX for if everything is working properly.

(Avoiding long TXs is actually pretty tricky. It requires performing no network blocking actions while holding a transaction.)

crawshaw avatar Dec 19 '18 19:12 crawshaw

The first easy thing we should do is pass context.Deadline from sqlite.Pool through to the returned connection, so that if user code wants to use a connection for more than 10 seconds, it does not get SQLITE_BUSY calls. (I still think infinite deadlines should be ignored because it's too easy to deadlock.)

I've been avoiding a direct context dependency on sqlite because that drags in reflect and the rest of the kitchen sink, which makes life harder on potential tinygo use. So I'm considering:

  1. Rename sqliteutil to sqlitex.
  2. Move sqlite.Pool to sqlitex.
  3. Replace the done <-chan struct{} parameter to Pool.Get with a context.Context.

crawshaw avatar Dec 20 '18 14:12 crawshaw

A second motivation for moving the sqlitex.Pool object to using a full context is that then it may be possible to use runtime/trace inside the sqlite package. (Though it will have to be done via some kind of adapter that removes the context dependency.)

crawshaw avatar Dec 20 '18 21:12 crawshaw

I'm playing with these API changes on https://github.com/crawshaw/sqlite/tree/sqlitex

crawshaw avatar Dec 20 '18 21:12 crawshaw

(I am a little surprised I would actually have 10s write transactions in there, although it was a 600GB database so I suppose it’s possible. I didn’t have the time to look into it yet unfortunately.)

FiloSottile avatar Dec 21 '18 01:12 FiloSottile

I'm hitting this on a really small database where I'm fairly certain that I'm not keeping transactions open and with no contention (single process). Any ideas what could be causing this?

EDIT: Sorry for the noise, I realize I mixed up ROLLBACK and ROLLBACK TO, so entirely user error.

zombiezen avatar Feb 08 '19 03:02 zombiezen

I'm hitting this without shared cache enabled. I'm definitely using conn for less than the busy timeout value, but I still get SQLITE_BUSY in some cases. With shared cache enabled it doesn't happen as far as I'm aware.

burdiyan avatar Dec 21 '21 17:12 burdiyan

One common source of this error code is during the upgrade from a read transaction to a write transaction. Switching to BEGIN IMMEDIATE for code that can contain a write cuts down on the likelihood of this error.

zombiezen avatar Dec 21 '21 20:12 zombiezen

@zombiezen thanks, I've been researching a lot this topic, and apparently this is indeed a solution. It's a bit inconvenient because the library doesn't offer anything to start the transaction easily. Similar to how SAVEPOINT can be started with Save() would be nice. Or did I miss it?

burdiyan avatar Dec 21 '21 21:12 burdiyan

There isn't anything in this library AFAIK that helps, but I did end up adding such a function in my fork: sqlitex.ImmediateTransaction.

zombiezen avatar Dec 21 '21 23:12 zombiezen