Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BlockOnBusy backoff period is long #75

Closed
wdullaer opened this issue Feb 2, 2024 · 5 comments
Closed

BlockOnBusy backoff period is long #75

wdullaer opened this issue Feb 2, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@wdullaer
Copy link

wdullaer commented Feb 2, 2024

I'm in the process of porting a codebase to use this library for its sqlite interactions.

We have a very highly concurrent workload and some tests that assert that the application doesn't start returning busy errors under load.

When I ported these tests over, they started taking over 3 mins to complete, while in the current implementation they take 1s at most.

I managed to trace down the difference to the backoff times used in the BlockOnBusy handler:

go-sqlite/sqlite.go

Lines 338 to 351 in 5c555a3

var busyDelays = [...]time.Duration{
1 * time.Second,
2 * time.Second,
5 * time.Second,
10 * time.Second,
15 * time.Second,
20 * time.Second,
25 * time.Second,
25 * time.Second,
25 * time.Second,
50 * time.Second,
50 * time.Second,
100 * time.Second,
}

The shortest wait period is 1s, and it increases very rapidly from there.

If we compare this with the backoff used in the default BusyTimeout handler:
https://gitlab.com/cznic/sqlite/-/blob/master/lib/sqlite_darwin_amd64.go?ref_type=heads#L121226
This one starts 1ms and waits up to 100ms at most.

I think there's too much difference between these two: a backoff of a few ms will suffice for most well behaved applications to resolve the lock.
In any case, it makes the current default behaviour of this library unusable in my project, it will tank throughput. I can revert to using a busy timeout, but I like the BlockOnBusy design: it removes a hard to guess tunable from the equation.

If you are not willing to change these backof timeouts, would you consider making them tunable with a config option?

@zombiezen zombiezen added the bug Something isn't working label Feb 2, 2024
@zombiezen
Copy link
Owner

Seems reasonable. I think I copied these values from somewhere in the SQLite source without much thought.

@ncruces
Copy link

ncruces commented Feb 2, 2024

Just FYI.

The SQLite implementation does use those numbers, but they're milliseconds, not seconds:
https://github.com/sqlite/sqlite/blob/3a32690a5519f7927947076bf17f64f4243c4396/src/main.c#L1713-L1753

sqlite3OsSleep takes microseconds, so delay in sqlite3OsSleep(db->pVfs, delay*1000) is in milliseconds; just below, sqlite3OsSleep(db->pVfs, 1000000) is sleeping a hole second.

So SQLite (by default, in all modern systems) never sleeps in increments of more than 100ms.
Even when it's sleeping for 60s, it'll poll the lock every 100ms.

@zombiezen
Copy link
Owner

Whoops, then that was definitely an error on my part. Thanks all!

@wdullaer
Copy link
Author

wdullaer commented Feb 5, 2024

Thanks for the quick turnaround!
I would have just stuck with the delay list of sqlite myself, there's something to be said for keeping this kind of behaviour equivalent, but it should be really hard to reach the 100ms+ delay in practice.
Again, thanks for the swift reply and all the work on the project. It's been a joy to use.

zombiezen added a commit that referenced this issue Feb 5, 2024
@zombiezen
Copy link
Owner

I would have just stuck with the delay list of sqlite myself, there's something to be said for keeping this kind of behaviour equivalent, but it should be really hard to reach the 100ms+ delay in practice.

You and @ncruces make a fair point. I've removed the values above 100ms. That will be incorporated in the next release.

Again, thanks for the swift reply and all the work on the project. It's been a joy to use.

You're very welcome! I am glad that is your experience. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants