penberg / limbo

Limbo is a work-in-progress, in-process OLTP database management system, compatible with SQLite.
MIT License
960 stars 53 forks source link

rusqlite benchmarks fail with "database locked" #217

Closed penberg closed 1 month ago

penberg commented 1 month ago

I guess we forget to drop the database lock for Limbo benchmarks:

Found 9 outliers among 100 measurements (9.00%)
  4 (4.00%) high mild
  5 (5.00%) high severe
Benchmarking rusqlite/Prepare statement: 'SELECT * FROM users LIMIT 1': Warming up for 3.0000 sthread 'main' panicked at core/benches/benchmark.rs:113:57:
called `Result::unwrap()` on an `Err` value: SqliteFailure(Error { code: DatabaseBusy, extended_code: 5 }, Some("database is locked"))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: bench failed, to rerun pass `-p limbo_core --bench benchmark`
penberg commented 1 month ago

@gvos94 Any ideas why the following patch is not enough to fix the benchmark? Are we not dropping the file lock when Database is dropped?

diff --git a/core/benches/benchmark.rs b/core/benches/benchmark.rs
index 9be7679..2fc08f3 100644
--- a/core/benches/benchmark.rs
+++ b/core/benches/benchmark.rs
@@ -89,6 +89,9 @@ fn bench(c: &mut Criterion) {
         },
     );

+    drop(conn);
+    drop(db);
+    drop(io);
     drop(group);

     // https://github.com/penberg/limbo/issues/174
brayanjuls commented 1 month ago

@gvos94 Any ideas why the following patch is not enough to fix the benchmark? Are we not dropping the file lock when Database is dropped?

diff --git a/core/benches/benchmark.rs b/core/benches/benchmark.rs
index 9be7679..2fc08f3 100644
--- a/core/benches/benchmark.rs
+++ b/core/benches/benchmark.rs
@@ -89,6 +89,9 @@ fn bench(c: &mut Criterion) {
         },
     );

+    drop(conn);
+    drop(db);
+    drop(io);
     drop(group);

     // https://github.com/penberg/limbo/issues/174

I think for this to work the Database and Connection struct need to implement the Drop trait.

penberg commented 1 month ago

@brayanjuls We don't need additional Drop traits because DarwinFile already has one, which calls unlock_file(). The reason you fixed the issue in #223 was because I forgot to drop stmt (which keeps a reference count to the file, preventing drop).

While your fix is much cleaner, this would have also worked:

@@ -89,6 +34,10 @@ fn bench(c: &mut Criterion) {
         },
     );

+    drop(stmt);
+    drop(conn);
+    drop(db);
+    drop(io);
     drop(group);

     // https://github.com/penberg/limbo/issues/174

Thanks for fixing the issue!