chore: sqlite framework#2265
Conversation
31dc8cc to
a954f57
Compare
| //! parameter and expand it with `json_each`, keeping the SQL text constant: | ||
| //! | ||
| //! - integer keys: `... WHERE col IN (SELECT value FROM json_each(?1))` | ||
| //! - BLOB keys: `... WHERE hex(col) IN (SELECT value FROM json_each(?1))` |
There was a problem hiding this comment.
I wonder what the cost of this hex() conversion is for BLOB values... I mean this would require one conversion per each list element in our query, but also in the SQLite query when it's comparing values, which might make this fairly expensive.
Do you have benchmarks for this?
|
|
||
| /// Per-connection prepared-statement cache capacity. Raised above rusqlite's default of 16 because | ||
| /// the store keeps a larger set of distinct statements. | ||
| const STATEMENT_CACHE_CAPACITY: usize = 64; |
There was a problem hiding this comment.
BTW, Diesel uses an "unlimited" cache -- we might as well just use something significantly larger here, like ~512 queries? We have a limited number of concurrent connections in the pool, so I think it's safe to do so.
| impl PinnedConnection { | ||
| /// Runs `query` inside a read-only (`DEFERRED`, never committed) transaction on the pinned | ||
| /// connection. | ||
| pub async fn read<R, E, F>(&self, msg: impl ToString + Send, query: F) -> Result<R, E> |
There was a problem hiding this comment.
As far as I understand this (and .write()) are the only ways to execute queries on a connections. These take a sync FnOnce that gets passed the actual ReadTx / WriteTx transaction.
This is very similar to what we had with Diesel.
My problem with this approach is that this makes it pretty much impossible to implement a "one transaction per request" model where the request handle could mix sync queries with async code. For that to work we'd need to have an API that actually returns a transaction instance the RPC handler can use while handling the same request.
There was a problem hiding this comment.
I concur with this; I think a rough concept is:
let (write_conn, read_only_conn_pool) = database.load(<path>).unwrap();This would also enforce that there is a single write connection only.
We're also largely "lying" about async in our current usage. The only actual async work we do is network IO over gRPC. The rest are all BS wrappers hiding the sync work - file IO, Sqlite and rocksdb are all sync operations in reality.
| /// A read-only transaction. Opened `DEFERRED` and never committed (changes roll back on drop). | ||
| pub struct ReadTx<'t>(&'t Transaction<'t>); | ||
|
|
||
| /// A read-write transaction. Opened `IMMEDIATE` and committed by the pool when the closure returns | ||
| /// `Ok`. | ||
| pub struct WriteTx<'t>(&'t Transaction<'t>); |
There was a problem hiding this comment.
If we reframe this as
a write transaction is a read transaction with additional permissions
then you can reduce the duplication by:
| /// A read-only transaction. Opened `DEFERRED` and never committed (changes roll back on drop). | |
| pub struct ReadTx<'t>(&'t Transaction<'t>); | |
| /// A read-write transaction. Opened `IMMEDIATE` and committed by the pool when the closure returns | |
| /// `Ok`. | |
| pub struct WriteTx<'t>(&'t Transaction<'t>); | |
| /// A read-only transaction. Opened `DEFERRED` and never committed (changes roll back on drop). | |
| pub struct ReadTx<'t>(&'t Transaction<'t>); | |
| /// A read-write transaction. Opened `IMMEDIATE` and committed by the pool when the closure returns | |
| /// `Ok`. | |
| pub struct WriteTx<'t>(ReadTx<'t>); | |
| // This lets you elide the entire read queries. | |
| impl Deref<ReadTx> for WriteTx { | |
| .. | |
| } |
| // SHARED QUERY HELPERS | ||
| // ================================================================================================= | ||
|
|
There was a problem hiding this comment.
As mentioned above, I think we can collapse all of this by using Deref.
There was a problem hiding this comment.
That said, I'm not sure that we need these helpers? Maybe we only need to expose a query method and see how that goes?
| mod errors; | ||
| mod manager; | ||
| pub mod migration; | ||
| pub mod sqlite; |
There was a problem hiding this comment.
Given that the end goal is for this to only be sqlite, I don't think we need to hide it in a submodule? i.e. we could just have transaction, pool, connection etc living in the root.
closes #2248 and #2249
Summary
Introduces a light SQLite framework in crates/db (
miden-node-db) and migrates the validator onto it, removing Diesel. It handles a connection pool, type-enforced read/write transactions, always-cached queries, a type codec, and cacheable IN-list helpers.handle:
read(...)gives you aReadTx. It begins a read-only transaction that is never committed, sonothing it does can persist.
write(...)gives you aWriteTx. It begins a write transaction.query_rows,query_opt,query_one,exists,countandexecute(write only). Every one of them prepares its statement through SQLite's cache.The usage is like this:
Changelog