db: flag slow transactions, add opt-in commit ledger - #729
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces transaction monitoring and accounting to track slow database transactions and record commit counts by domain call site. It adds warnings for transactions that stall during initiation or are held longer than a threshold, integrates with Go's runtime tracing, and provides a global ledger for transaction statistics. The review feedback suggests improving the robustness of the stack-walking logic in execTxCallerHint by reducing the hardcoded skip count in runtime.Callers to prevent fragility against compiler inlining or call-stack changes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // a flagged transaction. Only invoked on the slow path. | ||
| func execTxCallerHint() string { | ||
| pcs := make([]uintptr, 16) | ||
| n := runtime.Callers(3, pcs) |
There was a problem hiding this comment.
Using a hardcoded skip count of 3 in runtime.Callers is fragile because it assumes a specific call depth. If execTxCallerHint or recordTxnCommit are ever inlined by the compiler, or if the call path changes, the skip count of 3 can skip past the actual caller frame.
Since the loop already filters out any frames containing "darepo-client/db", we can safely use a smaller skip count like 1 (or 2 to skip only runtime.Callers and execTxCallerHint). The package-path filtering will then reliably stop at the first frame outside the db package, making the caller attribution completely robust against compiler optimizations and call-stack refactoring.
| n := runtime.Callers(3, pcs) | |
| n := runtime.Callers(1, pcs) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3bac84871
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if txnAccountingEnabled.Load() { | ||
| recordTxnCommit(txOptions.ReadOnly()) |
There was a problem hiding this comment.
When a store call runs under a durable-actor transaction, actor.TxFromContext returns at the top of ExecTx, so this newly added accounting hook is never reached. I checked baselib/actor/durable_actor.go and db/actordelivery/store_impl.go: actor message processing opens and commits its outer transaction in TxAwareActorDeliveryStore.ExecTx, then store methods invoked by the behavior join that transaction through the context. In actor-driven workloads, EnableTxnAccounting will therefore undercount write commits and omit the call sites that often dominate production transaction volume, making the benchmark ledger misleading.
Useful? React with 👍 / 👎.
897b8f4 to
bb9bb3b
Compare
d3bac84 to
610448a
Compare
b9242ad to
5a4b20d
Compare
2f8f9fe to
4f4b261
Compare
5a4b20d to
811adab
Compare
The stress workload intermittently surfaces SQLITE_BUSY on the send path even though busy_timeout is confirmed armed at 30s, which means some transaction occasionally holds the single-writer lock for tens of seconds. The CPU and block profiles cannot attribute that: the holder is waiting, not spinning, and database/sql pool waits hide the owning call site. In this commit, we teach ExecTx to identify both sides of a stall. A transaction whose body+commit exceeds one second logs a warning with the owning call site (the first caller frame outside the db packages, resolved only on the slow path), and a begin that waits longer than a second logs the victim side. When a runtime trace is active (the arktest --trace runs), every transaction also opens a trace region tagged with the same call site, so go tool trace shows exactly which goroutine sat inside a transaction across a stall window. The fast path cost is two clock reads and one IsEnabled check.
To pick the next write-amplification lever empirically instead of from the estimated commit audit, ExecTx can now attribute every committed transaction to the first caller frame outside the db packages, split by the read-only flag. The ledger is disabled by default and costs one atomic load per commit until a benchmark or test arms it, mirroring the slow-transaction telemetry's gating approach. The stress harness arms the ledger for the payment phase and divides the totals by attempted payments, validating the ~27-commits-per- payment audit and naming the dominant remaining buckets.
4f4b261 to
cfab0af
Compare
2 participants
In this PR, we add two opt-in observability hooks to the
dbpackage, bothborn during the OOR benchmarking campaign and both having already paid for
themselves in bugs found.
The first flags long-held transactions and stalled begins: any transaction
held past a threshold, or a begin that waits too long on the writer lock,
gets a warning with the caller attributed via the existing
execTxCallerHintframe walk. This is what caught the operator-sideingress fold holding the writer lock across synchronous handler work
(32-58s begin stalls) during development; without it that regression read
as a generic throughput collapse.
The second is a per-call-site commit ledger: when armed
(
EnableTxnAccounting), every successful commit is attributed to itsfirst non-db caller frame and bucketed by read/write. The cost when
disabled is a single atomic load per commit. The motivating result: an
audit had estimated ~27 write commits per payment; the ledger measured
77.9, and named the bucket every estimate had missed (the connection
actors' AckState cursor checkpoints, ~21/payment). Every subsequent
write-reduction lever in the train was driven by ledger deltas instead of
estimates. The stress harness arms it via
--txn-accountingand dumps atxn_ledger.txtinto the run artifacts (darepo side).This PR builds on #728 and is part of the OOR optimization train.