cockroachdb / cockroachdb/cockroach

kv,testserver: more easily inject transaction retries

Open
#106,417 1 comment 1 reaction 0 assignees View on GitHub
C-enhancement db-cy-23 O-postmortem P-3 T-kv
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

We've had a number of bugs caused by the incorrect management of state within a retriable function passed to the `(*sql.InternalDB).Txn` or `(*kv.DB.)Txn` methods.

For example, see the two recent issues:

- https://github.com/cockroachdb/cockroach/pull/104752
- https://github.com/cockroachdb/cockroach/pull/106378

We'd like to be able to more aggressively find these types of issues.

Currently, there are a few tools one can use to generate transaction retries in a test:

- `crdb_internal.force_retry`
- `SET inject_retry_errors_enabled=true`
- arranging for a write conflict on some unrelated scratch key via some well placed testing hook
https://github.com/cockroachdb/cockroach/blob/6a6ebcb950c050652ad8cded716dcd242e3a729c/pkg/sql/mvcc_backfiller_test.go#L568-L581
- using the TestingRequestFilter testing knob to inject a retriable error in response to EndTxn.
https://github.com/cockroachdb/cockroach/blob/6a6ebcb950c050652ad8cded716dcd242e3a729c/pkg/jobs/jobs_test.go#L2764-L2776

These work well if the code in question is designed to make such tools easy to use inside a test. However, they aren't great for finding transaction retry issues in existing code.

It would be nice to have a feature that could force all DB.Txn to retry with some probability without authors having to have pre-arranged for this to be possible.

When experimenting with a simple version of such a feature locally (#106506), it has already proven useful in potentially reproducing a rare bug that we've seen in CI and finding new issues. Including but not limited to:

*Non-trivial Issues*

- #106701
- #106503
- #106509

*Likely Trivial Issues*

- #106510
- #106808
- #106812

*Potential Issues*

- #106696
- #106700
- #106810
- rangefeed library calls unable to handle duplicate onSpanDone calls during initial scan (TODO: investigate)

*Next step*

Before we can turn something like this on by default, we need to flush out issues that would cause this to be too noisy for teams. The logictests provide one place where we can get a large amount of coverage without modifying a large number of tests:

- [ ] Make sure existing fixes above have been backported if appropriate.
- [ ] Add helper to make it a bit faster to write tests targetting particular transactions.
- [ ] Run against all logictests to identify more issues
- [ ] Add logictest feature to opt test out of random txn retires
- [ ] Enable by default in logictests

Jira issue: CRDB-29559

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.