cockroachdb / cockroachdb/cockroach
kv/kvpb: Error.checkTxnStatusValid decodes the same EncodedError twice
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Summary**
[`Error.checkTxnStatusValid()`](https://github.com/cockroachdb/cockroach/blob/bfb479b7e356f7fa1ff5d43257b9cf8d2a63debd/pkg/kv/kvpb/errors.go#L425-L440) decodes the same `EncodedError` twice in quick succession:
- [`e.GetDetail()`](https://github.com/cockroachdb/cockroach/blob/bfb479b7e356f7fa1ff5d43257b9cf8d2a63debd/pkg/kv/kvpb/errors.go#L393-L400) calls `errors.DecodeError(context.Background(), e.EncodedError)`.
- [`e.TransactionRestart()`](https://github.com/cockroachdb/cockroach/blob/bfb479b7e356f7fa1ff5d43257b9cf8d2a63debd/pkg/kv/kvpb/errors.go#L231-L239) calls `errors.DecodeError(context.Background(), e.EncodedError)` again on the same payload.
`checkTxnStatusValid` is invoked from `UpdateTxn`/`SetTxn`, which run on the DistSender response path. When the encoded error is a large `LockConflictError` (many lock spans), each `DecodeError` unmarshals a large protobuf, so paying for it twice per error is pure waste.
This is a concrete instance of the broader audit tracked in #143445.
**Evidence**
From a CPU profile taken during a customer incident (v25.4.8, node under heavy lock contention), `checkTxnStatusValid` accounted for **13.2% of total CPU** on the node, split almost evenly between the two redundant decodes:
```
(*Error).checkTxnStatusValid 0 37.50s (flat, cum) 13.20% of Total
├─ (*Error).GetDetail 19.18s 51.15%
└─ (*Error).TransactionRestart 18.30s 48.80%
```
Both children bottom out in `errors.DecodeError → LockConflictError.Unmarshal`. Decoding once would roughly halve this (~6.5% of node CPU during the incident).
**Suggested fix**
Decode the error once in `checkTxnStatusValid` and feed the decoded `error` to both checks — e.g. extract decode-free helpers (`getDetail(err)` / `transactionRestart(err)`) that operate on an already-decoded error, and have `checkTxnStatusValid` call `errors.DecodeError` a single time. The public `GetDetail()`/`TransactionRestart()` can wrap those helpers.
**Next steps**
- [ ] Refactor `checkTxnStatusValid` to decode once.
- [ ] Consider whether other call sites that pair can share a single decode.
Epic CRDB-48840
Jira issue: CRDB-64635
Contributor guide
Assessment
This issue has not been assessed yet.