sql: the key reported by contention events is incorrect

[arulajmani]

Describe the problem

The key reported in a contention event is supposed to be the key on which there was a conflict, as per the comment here:

cockroach/pkg/kv/kvpb/api.proto

Lines 3661 to 3665 in 41c37cb

	message ContentionEvent {
	option (gogoproto.goproto_stringer) = false;
	// Key is the key that this and the other transaction conflicted on.
	bytes key = 1 [(gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/roachpb.Key"];

However, it's populated here:

cockroach/pkg/sql/conn_executor_exec.go

Lines 3365 to 3366 in c88a1ce

	BlockingEvent: kvpb.ContentionEvent{
	Key: retryErr.ConflictingTxn.Key,

In doing so, we're using the key where the conflicting transaction's transaction record is anchored. See:

cockroach/pkg/storage/enginepb/mvcc3.proto

Lines 29 to 33 in c097c19

	// key is the key which anchors the transaction. This is typically
	// the first key read or written during the transaction and
	// determines which range in the cluster will hold the transaction
	// record.
	bytes key = 3; // TODO(tschottdorf): [(gogoproto.casttype) = "Key"];

This doesn't necessarily mean that this was the key the two transactions conflicted on; just that one of the transactions wrote to this key. This seems unintentional.

Jira issue: CRDB-39609

[blathers-crl]

Hi @arulajmani, please add branch-* labels to identify which branch(es) this C-bug affects.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[arulajmani]

cc @kevin-v-ngo -- what's the right board to put this issue on?

[tbg]

Some more color on how the key should be propagated. Full Cursor chat (CRL only).

Fix: Use actual conflict key (not txn anchor key) in serialization conflict contention events

Problem

When a serialization conflict causes a transaction retry, the resulting ExtendedContentionEvent incorrectly reports the conflicting transaction's anchor key instead of the actual key where the conflict occurred.

cockroach/pkg/sql/conn_executor_exec.go

Lines 4439 to 4440 in 4ad50ea

	Key: retryErr.ConflictingTxn.Key,
	TxnMeta: *retryErr.ConflictingTxn,

The code uses retryErr.ConflictingTxn.Key, but TxnMeta.Key is the transaction's anchor key (the first key the transaction wrote to), not the key where the serialization conflict was detected.

Root Cause

The actual conflict key is available in the RefreshFailedError (field Key) but is discarded when constructing the TransactionRetryError:

cockroach/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go

Lines 501 to 523 in 4ad50ea

	func newRetryErrorOnFailedPreemptiveRefresh(
	txn *roachpb.Transaction, pErr *kvpb.Error,
	) *kvpb.Error {
	var conflictingTxn *enginepb.TxnMeta
	msg := redact.StringBuilder{}
	msg.SafeString("failed preemptive refresh")
	if pErr != nil {
	if refreshErr, ok := pErr.GetDetail().(*kvpb.RefreshFailedError); ok {
	if refreshErr.ConflictingTxn != nil {
	conflictingTxn = refreshErr.ConflictingTxn
	}
	msg.Printf(" due to %s", refreshErr)
	} else if wiErr, ok := pErr.GetDetail().(*kvpb.WriteIntentError); ok {
	if len(wiErr.Locks) > 0 {
	conflictingTxn = &wiErr.Locks[0].Txn
	}
	msg.Printf(" due to %s", wiErr)
	} else {
	msg.Printf(" - unknown error: %s", pErr)
	}
	}
	retryErr := kvpb.NewTransactionRetryError(kvpb.RETRY_SERIALIZABLE, msg.RedactableString(), kvpb.WithConflictingTxn(conflictingTxn))
	return kvpb.NewErrorWithTxn(retryErr, txn)

The function extracts ConflictingTxn from either:

• RefreshFailedError.ConflictingTxn (key available as refreshErr.Key)
• WriteIntentError.Locks[0].Txn (key available as wiErr.Locks[0].Key)

But the actual conflict key is never captured.

Implementation Plan

1. Add ConflictingKey field to proto messages

File: pkg/kv/kvpb/errors.proto

Add a new field to TransactionRetryError:

cockroach/pkg/kv/kvpb/errors.proto

Lines 248 to 261 in 4ad50ea

	message TransactionRetryError {
	optional TransactionRetryReason reason = 1 [(gogoproto.nullable) = false];
	// Note: Deprecated. `extra_msg_redactable` takes precedence if present.
	// Both fields will be populated in 23.1.
	// TODO(davidh): Remove in 23.2
	optional string extra_msg = 2 [(gogoproto.nullable) = false];
	optional string extra_msg_redactable = 3 [(gogoproto.nullable) = false, (gogoproto.customtype) = "github.com/cockroachdb/redact.RedactableString"];
	// The conflicting transaction's TxnMeta. This is bubbled up from a
	// RefreshFailedError. This field is only set if the retry error is
	// constructed in response to a RefreshFailedError. In all other cases, or if
	// the RefreshFailedError does not contain conflicting transaction
	// information, this field is unset.
	optional storage.enginepb.TxnMeta conflicting_txn = 4;
	}

// Add after conflicting_txn field:
optional bytes conflicting_key = 5 [(gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/roachpb.Key"];

Add a new field to TransactionRetryWithProtoRefreshError:

cockroach/pkg/kv/kvpb/errors.proto

Lines 508 to 568 in 4ad50ea

	message TransactionRetryWithProtoRefreshError {
	// A user-readable message.
	// Note: Deprecated in favor of `msg_redactable` below. Both fields will be
	// populated in 23.1. Presence of `msg_redactable` will take precedence.
	// TODO(davidh): Remove in 23.2
	optional string msg = 1 [(gogoproto.nullable) = false];
	// The ID of the transaction being retried. The client can compare this ID
	// that of the new transaction (below) to check whether the transaction was
	// aborted and will restart from the beginning with a new identity.
	optional bytes prev_txn_id = 2 [
	(gogoproto.customtype) = "github.com/cockroachdb/cockroach/pkg/util/uuid.UUID",
	(gogoproto.customname) = "PrevTxnID",
	(gogoproto.nullable) = false];
	// The epoch of the transaction being retried. The client can compare this
	// epoch that of the new transaction (below) to check whether the transaction
	// is being restarted from the beginning with all writes discarded.
	optional int32 prev_txn_epoch = 5 [
	(gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/storage/enginepb.TxnEpoch",
	(gogoproto.nullable) = false];
	// The Transaction that should be used by next attempts. Depending on the
	// original cause of this method and the isolation level of the transaction,
	// this can be:
	//
	// - the same Transaction as before (same ID), but with an incremented epoch
	// and read timestamp. The transaction must start again from the beginning,
	// with all writes discarded. This case applies to isolation levels that do
	// not use a per-statement read snapshot, and must therefore start from the
	// beginning if the read snapshot must be adjusted.
	//
	// - the same Transaction as before (same ID) with the same epoch as before,
	// but with an incremented read timestamp. The transaction can proceed without
	// the loss of any of its writes, although clients may employ savepoints to
	// selectively discard the writes from the read snapshot scope (i.e. the
	// statement) being retried. This case applies to isolation levels that use a
	// per-statement read snapshot, and can therefore retry select portions of the
	// transaction if the read snapshot must be adjusted, without starting from
	// the beginning.
	//
	// - a completely new Transaction (different ID). The transaction must start
	// again from the beginning with all writes discarded and with a new identity.
	// This case applies to all isolation levels. Transaction aborts are typically
	// associated with locking deadlocks where the previous transaction's locks
	// were discarded to break a deadlock between transactions and also with cases
	// where the kv layer determines that it has lost information cannot process a
	// certain transaction.
	optional roachpb.Transaction next_transaction = 3 [(gogoproto.nullable) = false];
	// A user-readable message containing redaction markers.
	optional string msg_redactable = 4 [(gogoproto.nullable) = false, (gogoproto.customtype) = "github.com/cockroachdb/redact.RedactableString"];
	// The conflicting transaction's TxnMeta. This field is bubbled up from a
	// RefreshFailedError. This field is only set when a
	// TransactionRetryWithProtoRefreshError is constructed in response to a
	// RefreshFailedError which includes information about the conflicting
	// transaction that caused the refresh to fail. In all other cases this field
	// is unset
	optional storage.enginepb.TxnMeta conflicting_txn = 6;
	}

// Add after conflicting_txn field:
optional bytes conflicting_key = 7 [(gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/roachpb.Key"];

2. Add WithConflictingKey option

File: pkg/kv/kvpb/errors.go

Add a new option function similar to WithConflictingTxn:

cockroach/pkg/kv/kvpb/errors.go

Lines 721 to 743 in 4ad50ea

	type retryErrOptions struct {
	conflictingTxn *enginepb.TxnMeta
	}
	// RetryErrOption is used to annotate optional fields in retry related errors.
	type RetryErrOption interface {
	apply(*retryErrOptions)
	}
	type retryErrOptionFunc func(*retryErrOptions)
	func (f retryErrOptionFunc) apply(o *retryErrOptions) {
	f(o)
	}
	// WithConflictingTxn is used to annotate a retry error with the conflicting
	// transaction metadata (optional). This is only used for cases where a refresh
	// fails with `REASON_INTENT`.
	func WithConflictingTxn(txn *enginepb.TxnMeta) RetryErrOption {
	return retryErrOptionFunc(func(o *retryErrOptions) {
	o.conflictingTxn = txn
	})
	}

• Extend retryErrOptions struct to include conflictingKey roachpb.Key
• Add WithConflictingKey(key roachpb.Key) RetryErrOption function
• Update NewTransactionRetryError to set ConflictingKey from options
• Update NewTransactionRetryWithProtoRefreshError to set ConflictingKey from options

3. Capture conflict key in span refresher

File: pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go

Update newRetryErrorOnFailedPreemptiveRefresh:

cockroach/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go

Lines 501 to 524 in 4ad50ea

	func newRetryErrorOnFailedPreemptiveRefresh(
	txn *roachpb.Transaction, pErr *kvpb.Error,
	) *kvpb.Error {
	var conflictingTxn *enginepb.TxnMeta
	msg := redact.StringBuilder{}
	msg.SafeString("failed preemptive refresh")
	if pErr != nil {
	if refreshErr, ok := pErr.GetDetail().(*kvpb.RefreshFailedError); ok {
	if refreshErr.ConflictingTxn != nil {
	conflictingTxn = refreshErr.ConflictingTxn
	}
	msg.Printf(" due to %s", refreshErr)
	} else if wiErr, ok := pErr.GetDetail().(*kvpb.WriteIntentError); ok {
	if len(wiErr.Locks) > 0 {
	conflictingTxn = &wiErr.Locks[0].Txn
	}
	msg.Printf(" due to %s", wiErr)
	} else {
	msg.Printf(" - unknown error: %s", pErr)
	}
	}
	retryErr := kvpb.NewTransactionRetryError(kvpb.RETRY_SERIALIZABLE, msg.RedactableString(), kvpb.WithConflictingTxn(conflictingTxn))
	return kvpb.NewErrorWithTxn(retryErr, txn)
	}

• Add a conflictingKey variable
• For RefreshFailedError: set conflictingKey = refreshErr.Key
• For WriteIntentError: set conflictingKey = wiErr.Locks[0].Key
• Pass kvpb.WithConflictingKey(conflictingKey) to NewTransactionRetryError

4. Propagate conflict key through TxnCoordSender

File: pkg/kv/kvclient/kvcoord/txn_coord_sender.go

Update handleRetryableErrLocked:

cockroach/pkg/kv/kvclient/kvcoord/txn_coord_sender.go

Lines 879 to 929 in 4ad50ea

	var conflictingTxn *enginepb.TxnMeta
	switch tErr := pErr.GetDetail().(type) {
	case *kvpb.TransactionRetryError:
	conflictingTxn = tErr.ConflictingTxn
	switch tErr.Reason {
	case kvpb.RETRY_WRITE_TOO_OLD:
	tc.metrics.RestartsWriteTooOld.Inc()
	case kvpb.RETRY_SERIALIZABLE:
	tc.metrics.RestartsSerializable.Inc()
	case kvpb.RETRY_ASYNC_WRITE_FAILURE:
	tc.metrics.RestartsAsyncWriteFailure.Inc()
	case kvpb.RETRY_COMMIT_DEADLINE_EXCEEDED:
	tc.metrics.RestartsCommitDeadlineExceeded.Inc()
	default:
	tc.metrics.RestartsUnknown.Inc()
	}
	case *kvpb.WriteTooOldError:
	tc.metrics.RestartsWriteTooOld.Inc()
	case *kvpb.ReadWithinUncertaintyIntervalError:
	tc.metrics.RestartsReadWithinUncertainty.Inc()
	case *kvpb.ExclusionViolationError:
	tc.metrics.RestartsExclusionViolation.Inc()
	case *kvpb.TransactionAbortedError:
	tc.metrics.RestartsTxnAborted.Inc()
	case *kvpb.TransactionPushError:
	tc.metrics.RestartsTxnPush.Inc()
	default:
	tc.metrics.RestartsUnknown.Inc()
	}
	// Construct the next txn proto using the provided error.
	prevTxn := pErr.GetTxn()
	nextTxn, assertErr := kvpb.PrepareTransactionForRetry(pErr, tc.mu.userPriority, tc.clock)
	if assertErr != nil {
	return assertErr
	}
	// Construct the TransactionRetryWithProtoRefreshError using the next txn proto.
	retErr := kvpb.NewTransactionRetryWithProtoRefreshError(
	redact.Sprint(pErr), /* msg */
	prevTxn.ID, /* prevTxnID */
	prevTxn.Epoch, /* prevTxnEpoch */
	nextTxn, /* nextTxn */
	kvpb.WithConflictingTxn(conflictingTxn),
	)

• Extract conflictingKey from TransactionRetryError alongside conflictingTxn
• Pass kvpb.WithConflictingKey(conflictingKey) to NewTransactionRetryWithProtoRefreshError

5. Use conflict key in contention event

File: pkg/sql/conn_executor_exec.go

Update the contention event creation:

cockroach/pkg/sql/conn_executor_exec.go

Lines 4435 to 4440 in 4ad50ea

	if errors.As(txnErr, &retryErr) && retryErr.ConflictingTxn != nil {
	contentionEvent := contentionpb.ExtendedContentionEvent{
	ContentionType: contentionpb.ContentionType_SERIALIZATION_CONFLICT,
	BlockingEvent: kvpb.ContentionEvent{
	Key: retryErr.ConflictingTxn.Key,
	TxnMeta: *retryErr.ConflictingTxn,

• Change the condition to require both retryErr.ConflictingKey != nil and retryErr.ConflictingTxn != nil
• Use retryErr.ConflictingKey for the Key field in the ContentionEvent
• Keep using retryErr.ConflictingTxn for the TxnMeta field (the conflicting transaction's metadata is still correct/useful)
• Do not fall back to ConflictingTxn.Key — if ConflictingKey is not set, skip emitting the contention event rather than emit incorrect data

6. Regenerate protobufs

Run ./dev generate protobuf after proto changes.

Testing

• Add unit test in pkg/kv/kvpb/errors_test.go verifying ConflictingKey is preserved through the error chain
• Add/update integration test that triggers a serialization conflict and verifies the contention event reports the correct key