search

crawshaw / sqlite

Go SQLite3 driver
ISC License
572 stars 68 forks source link

Handle SQLITE_BUSY gracefully #47

Open FiloSottile opened 5 years ago

FiloSottile commented 5 years ago

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.

crawshaw commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

FiloSottile commented 5 years ago

(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.)

zombiezen commented 5 years ago

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.

burdiyan commented 2 years ago

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.

zombiezen commented 2 years ago

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.

burdiyan commented 2 years ago

@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?

zombiezen commented 2 years ago

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