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

Activate the WAL mode after creating the database file #54

Merged
merged 3 commits into from
Nov 1, 2022

Conversation

Halbaroth
Copy link
Contributor

This PR activates the WAL mode in the output database file.
Without activating this mode, it would be unsafe to read and write at the same time in the database.
The mode is valuable to people who want to read the database with an external program before the end of the execution of benchpress.

According to the above documentation, the WAL mode reduces slightly the performances of Sqlite but the difference is not
significant.

I didn't add an option to activate or deactivate this mode but I can add it if you think it is a better design.

@c-cube
Copy link
Member

c-cube commented Nov 1, 2022

Without activating this mode, it would be unsafe to read and write at the same time in the database.

I don't think that's correct 🙂 . The mutex is there to protect the DB. All that WAL does is allow concurrent writes, so it scales better.

I think it can be useful to enable WAL if you always use the same local DB file. An option to enable it is welcome. However, another use case (mine 😅) is to sometimes run benchpress on a big machine, and copy the DB afterwards, which is trickier with WAL because the DB is no longer a single file, but a bundle of 2 or 3 files.

@Halbaroth
Copy link
Contributor Author

If you copy the database while Benchpress is writing something, you could get a corrupted copy of it.
I tried to read the DB file with Caqti but I got errors because Benchpress locks the DB file when it is writing.
The only proper solution I found is to turn on the WAL mode. For instance:
https://www.skoumal.com/en/parallel-read-and-write-in-sqlite/
I might have missed something.

Another solution would be to provide a complete RESTful api backend, I really consider to write such a backend.

@c-cube
Copy link
Member

c-cube commented Nov 1, 2022

You need to setup the retry thingie (it's somewhere in benchpress 😅) in presence of the lock, yes :-). I only copy the DB after benchpress is done with it (when a benchmark is done, basically) so it's safe, but with WAL it might still have a journal file on the side, which is annoying.

An API is also a good idea, either via json or via something protobuf-based (to get schemas and code generation for free).

@Halbaroth
Copy link
Contributor Author

In my usage scenario, the benchmark is to long. Maybe we could setup the retry feature instead of the WAL mode?

@c-cube
Copy link
Member

c-cube commented Nov 1, 2022

the "retry feature" is Sqlite3.busy_timeout <n> where n is the number of milliseconds it'll retry a query that fails to acquire the lock. It's done per connection, and in benchpress it's setup via the ?timeout parameter of Sqlite3_utils.with_db. :).

Again I'm also open to having a CLI flag or some form of config for WAL! :)

@Halbaroth
Copy link
Contributor Author

Halbaroth commented Nov 1, 2022

Actually, I used a similar feature in my client with Caqti without WAL mode. It seems to work but I wouldn't use it because I feared that Benchpress could fail to acquire the connection lock.

So we can add two cli options:

  • an option to turn on the WAL mode. By default, the WAL mode is deactivated.
  • an option to set the timeout.

Concerning the api backend, I will respond to your issue #25.

@c-cube
Copy link
Member

c-cube commented Nov 1, 2022

I think only the first option is required 🙂 . The timeout is already set up internally (I just added it to a missing case). The option for WAL is definitely useful! 👍

@c-cube c-cube merged commit 2b1019e into sneeuwballen:master Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants