kv,testserver: more easily inject transaction retries

[stevendanna]

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:

• upgrades: fix txn retry bug in upgrade batching #104752
• upgrades: fix txn retry bug in upgrade batching #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
  

  cockroach/pkg/sql/mvcc_backfiller_test.go

  Lines 568 to 581 in 6a6ebcb

  	RunDuringMergeTxn: func(ctx context.Context, txn *kv.Txn, startKey roachpb.Key, endKey roachpb.Key) error {
  	var err error
  	retryOnce.Do(func() {
  	t.Logf("forcing txn retry for [%s, %s)] using %s", startKey, endKey, scratch)
  	if _, err := txn.Get(ctx, scratch); err != nil {
  	require.NoError(t, err)
  	}
  	require.NoError(t, kvDB.Put(ctx, scratch, "foo"))
  	// This hits WriteTooOldError, but we swallow the error to test the txn failing after the entire txn closure has run.
  	_ = txn.Put(ctx, scratch, "bar")
  	})
  	return err
  	},

• using the TestingRequestFilter testing knob to inject a retriable error in response to EndTxn.
  

  cockroach/pkg/jobs/jobs_test.go

  Lines 2764 to 2776 in 6a6ebcb

  	TestingRequestFilter: func(ctx context.Context, r *kvpb.BatchRequest) *kvpb.Error {
  	if r.Txn == nil || r.Txn.Name != txnName {
  	return nil
  	}
  	if _, ok := r.GetArg(kvpb.EndTxn); ok {
  	if !haveInjectedRetry {
  	haveInjectedRetry = true
  	// Force a retry error the first time.
  	return kvpb.NewError(kvpb.NewTransactionRetryError(kvpb.RETRY_REASON_UNKNOWN, "injected error"))
  	}
  	}
  	return nil
  	},

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

• upgrades: fix transaction retry error #106701
• userfile: fix transaction retry upload bug #106503
• upgrades: fix transaction retry bug in auto config job migration #106509

Likely Trivial Issues

• backupccl: fix txn retry bug during planning #106510
• ttljob: fix minor book-keeping bug during txn retry #106808
• stmtdiagnostics: fix upload in face of transaction retry #106812

Potential Issues

• sql: CREATE TABLE AS and REFRESH MATERIALIZED VIEW with some expressions fail in the face of transaction retries #106696
• jobs: scheduled jobs Create and Update functions are not txn retry safe #106700
• sql: CREATE STATISTICS can produce more than 1 stats entry in the face of rare transaction retry #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

[jbowens]

With Go generics available, it seems feasible to add variants of these that are generic over an input type and an output type. Then we could write a linter enforcing that invocations to these variants don't pass funcs that close over any outside state.

There's still the possibility of accidentally mutating state through pointers in the input type, but it's at least easier to audit too since you only need to look at the input type and what's reachable through it.