cockroachdb / cockroachdb/cockroach

kv/kvpb: Error.checkTxnStatusValid decodes the same EncodedError twice

Open
#171,516 1 comment 0 reactions 0 assignees View on GitHub
A-kv C-performance E-starter O-agent O-postmortem O-support P-3 T-kv
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.