mirror of
https://github.com/openobserve/openobserve.git
synced 2026-11-04 16:09:05 +00:00
Hengfei Yang
ccd254ad00
Paired with o2-enterprise `refactor/orm-rw-ro-client` (same branch
name).
## Problem
Every ORM caller went through a single client backed by the sqlite
**write** pool, so writes were kept apart only by `table::get_lock()` —
a convention 365 call sites had to remember. Forgetting it surfaced as
`(code: 517) database is locked`: a deferred transaction that reads and
then upgrades to a write finds the snapshot moved under it, and sqlite
reports that **without ever consulting `busy_timeout`**, which is why
the failures were immediate rather than retried.
## Approach
Serialization now comes from the pool, not from a lock. The sqlite write
pool holds **one connection** — all sqlite allows anyway — so writers
queue on acquire and there is nothing to forget.
That is only affordable because reads move to the read-only pool.
Measured, same load, fresh data dir each run:
| | reads on the write connection | reads on the RO pool |
|---|---|---|
| 200 concurrent reads | p50 **135 ms** | p50 **1 ms** |
| writes during mixed load | p50 **189 ms** | p50 **14 ms** |
Both were 100% success with zero lock errors, so this is purely
throughput — but without the split, reads queue behind the single writer
and drag writes down with them.
## API
`infra::db` now exposes three functions and nothing else:
```rust
pub async fn get_orm_client_ro() -> &'static DatabaseConnection
pub async fn get_orm_client_rw() -> &'static DatabaseConnection
pub async fn get_orm_client_ddl() -> &'static DatabaseConnection
```
The statics behind them, every `connect_to_orm_*`, and the raw
sqlite/postgres pools are now private or `pub(crate)`. `ORM_CLIENT`,
`connect_to_orm` and `table::get_lock()` are gone, as is the raw
`CLIENT_RW` mutex (56 lock acquisitions, plus the 52 guard derefs that
existed only to reach through it).
Final split: **RO 247 / RW 360 / DDL 9**; `get_lock` 0, `ORM_CLIENT` 0,
`CLIENT_RW.lock()` 0.
## How read/write was decided
Per call site, then verified by resolving each call through **its own
file's imports** (bare-name matching is useless here — `get_by_id`,
`list` and `update` collide across modules) and propagating write-ness
along the edges that hand a connection to a callee.
A wrong write client only costs throughput; a wrong read client fails in
production. So the audit is zero-miss in the read direction, and a site
is only downgraded when *every* connection-passing callee resolves.
## Behaviour changes on sqlite
All three make sqlite match what postgres already does today — postgres
has never had this lock:
- check-then-write pairs like `folders::put` are no longer atomic; they
rely on their unique index (`folders_org_folder_id_idx`)
- `db::put` runs two sequential transactions, so concurrent puts to one
key are last-write-wins
- `db::delete` runs several statements outside a transaction
`write_pool_is_single_connection_on_sqlite` is now the **only** guard
against reintroducing 517 — please don't drop it.
## Review guide
The mutex removal is separable if you want to look at it on its own:
`src/infra/src/{db,file_list,scheduler,pipeline,schema/history}/sqlite.rs`.
Everything else is the client split.
## Verification
- `cargo check --workspace --all-targets` and the CI clippy command:
clean
- Tests match a HEAD baseline built with the same `.env`, exactly: infra
1410/2, core 1629/1, db 431/0, api-management 179/0. The three failures
are pre-existing and environmental (two need a postgres that isn't
running locally)
- Static scans, all zero: queries on the pool inside an open
transaction; transactions spanning a second connection acquisition;
ro/rw misclassification
- Runtime smoke on a fresh data dir: 200 concurrent reads, then 100
reads + 100 writes simultaneously — 400/400 HTTP 200, rows persisted,
**0** lock errors, 0 ERROR/WARN lines
- Enterprise verified via the `Cargo.toml.openobserve` swap (`cargo
check --all-targets`): clean
Also fixes `users::tests::set_up`, which truncated shared tables but
released its lock before the test body ran, letting a concurrent test
clear rows mid-test.
https://claude.ai/code/session_016ouf1RxiMpiRw5W7GvCUDh