release-23.1: kv, storage: fix handling of ambiguous failures on commit

[AlexTalks]

Backport:

• 1/1 commits from storage: refactor mvcc write parameters #107680
• 1/1 commits from kvcoord: add test for transaction unexpectedly committed #107323
• 1/1 commits from kvcoord: refactor ambiguous commit tests #108154
• 1/1 commits from kvcoord: test additional ambiguous failure states #108001
• 1 new commit: kvcoord: fix ambiguous commit tests for 23.1 behavior
• 1/1 commits from kv: enable replay protection for ambiguous writes on commits #107658
• 1 new commit: kvcoord: modify ambiguous commit tests for WTO deferral on 23.1

Please see individual PRs for details.

/cc @cockroachdb/release

---

Release justification: Backporting fix for outstanding bug described in #103817.

[blathers-crl]

Thanks for opening a backport.

Please check the backport criteria before merging:

• Patches should only be created for serious issues or test-only changes.
• Patches should not break backwards-compatibility.
• Patches should change as little code as possible.
• Patches should not change on-disk formats or node communication protocols.
• Patches should not add new functionality.
• Patches must not add, edit, or otherwise modify cluster versions; or add version gates.

If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.

• There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
• The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
• New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
• The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

• What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
• Will this work in a cluster of mixed patch versions? Did we test that?
• If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed 80 of 80 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks and @jbowens)

---

pkg/kv/kvserver/logstore/logstore.go line 36 at r1 (raw file):

	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
	"github.com/cockroachdb/errors"
	"go.etcd.io/raft/v3"

Did you mean to remove this import? From the diff, I don't see what it would be removed.

---

pkg/kv/kvserver/logstore/stateloader.go line 185 at r1 (raw file):

	ctx context.Context, writer storage.Writer, replicaID roachpb.ReplicaID,
) error {
<<<<<<< HEAD

Cherry-pick skew?

---

pkg/storage/engine_test.go line 1606 at r1 (raw file):

	// However, certain clearers cannot clear intents, range keys, or point keys.
	writeInitialData := func(t *testing.T, rw ReadWriter) {
		var localTS hlc.ClockTimestamp

Should we just delete localTS?

[AlexTalks]

It is possible we may require backporting #102808 and #102809 in order for the tests (at least, those that depend on WriteTooOld errors) to work, otherwise we will need to remove them for 23.1. @nvanbenschoten let me know if you have some thoughts on this?

[AlexTalks]

As discussed on Slack, I've updated some of the tests to use CPuts instead of Puts, but unfortunately it is somewhat clear (especially in the last commit) that without #102808 and #102809, any time a retried blind write happens after the initial intents have been cleaned up, WTO deferral will mean we can't fully avoid getting transaction unexpectedly committed without further changes.

It feels like we have a few options from here:

• Proceed with backporting as is, knowing that there are some cases of the bug that won't be fully fixed on 23.1.
• Also backport kv: eliminate write-too-old deferral mechanism #102808 and storage: don't write intents on write-too-old errors #102809, if we feel there isn't significant risk associated.
• Make additional changes to the DistSender, something like:

br, err = transport.SendNext(ctx, ba)
...
if err == nil {
  if br.Error == nil {
    if ambiguousError != nil && br.Txn != nil && br.Txn.WriteTooOld {
      return nil, kvpb.NewAmbiguousResultErrorf("error=%v [propagate] (last error: WriteTooOld)", ambiguousError)
    }
  }
}

I'm not sure if it would be ok to leave around the intent written on the retry though, or if additional handling would be necessary?

@nvanbenschoten feel free to let me know if you have some thoughts or preferences here!

[nvanbenschoten]

Also backport #102808 and #102809, if we feel there isn't significant risk associated.

These changes feel too large and too risky to be backported. They also comes with some user-visible behavior changes that aren't appropriate for a patch release.

Make additional changes to the DistSender, something like ...

This seems like a reasonable change, although making it will require additional testing and introduces some additional risk for the backport. How do you feel about it?

Independent of that, we should get to the bottom of the flakiness in #110187 before backporting the flaky test to other release branches.

[AlexTalks]

@nvanbenschoten agreed. I actually put up a draft of the fix I am proposing in #111017.

[AlexTalks]

Closing this in favor of #111017 .

[daniel-crlabs]

Affects this customer: https://cockroachdb.zendesk.com/agent/tickets/19344