contention: Increase RetryBudgetForMissingResult

Previously the retry budget was set to 1, however this budget lead to
a significant amount of failed resolutions.

To see why a retry budget of 1 is not sufficient consider the case where an
in progress transaction is in the writer buffer when resolution is attempted.
The in progress txn is then ingested into the cache after the txn resolution
endpoint drains the write buffer - i.e. it is stored in the cache with the
appstatspb.InvalidTransactionFingerprintID value.

Now the transaction finishes and its respective fingerprint ID is recorded.
However, it is in the writer buffer of the txn id cache. When resolution is
attempted again, the lookup gets the invalid / in-progress value that is stored
in the cache. The subsequent flush then gets the cache to ingest the actual
fingerprint ID value for the txn. But we've run out of budget, and don't retry
resolution.

This commit increases the budget to 2. In addition to handling the case above,
experimentally it shows to lower the number of failed resolutions (see issue
linked).

Lastly, this commit removes dead code in the TxnID resolution endpoint. A map
was being created and never added to. The logic resulted in the RPC flushing
the TxnID Cache on every invocation, that behaviour is preserved and made
more explicit.

Fixes: #148686
Release note: None

Author: alyshanjahani-crl

pkg/server/status.go

@@ -86,7 +86,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing/tracingpb"
 	"github.com/cockroachdb/cockroach/pkg/util/uint128"
-	"github.com/cockroachdb/cockroach/pkg/util/uuid"
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/redact"
 	"github.com/google/pprof/profile"
@@ -416,11 +415,6 @@ func (b *baseStatusServer) localTxnIDResolution(
 ) *serverpb.TxnIDResolutionResponse {
 	txnIDCache := b.sqlServer.pgServer.SQLServer.GetTxnIDCache()
 
-	unresolvedTxnIDs := make(map[uuid.UUID]struct{}, len(req.TxnIDs))
-	for _, txnID := range req.TxnIDs {
-		unresolvedTxnIDs[txnID] = struct{}{}
-	}
-
 	resp := &serverpb.TxnIDResolutionResponse{
 		ResolvedTxnIDs: make([]contentionpb.ResolvedTxnID, 0, len(req.TxnIDs)),
 	}
@@ -434,12 +428,10 @@ func (b *baseStatusServer) localTxnIDResolution(
 		}
 	}
 
-	// If we encounter any transaction ID that we cannot resolve, we tell the
-	// txnID cache to drain its write buffer (note: The .DrainWriteBuffer() call
-	// is asynchronous). The client of this RPC will perform retries.
-	if len(unresolvedTxnIDs) > 0 {
-		txnIDCache.DrainWriteBuffer()
-	}
+	// Note(alyshan): TxnIDResolution is only called by the contention event resolver today.
+	// The resolver relies on these resolution calls to trigger a drain of the writer buffer
+	// on the txn id cache.
+	txnIDCache.DrainWriteBuffer()
 
 	return resp
 }

pkg/sql/contention/resolver.go

@@ -73,7 +73,7 @@ const (
 	//    in-memory data corruption (shouldn't happen in normal circumstances,
 	//    since access to txnID cache is all synchronized). In this case, no
 	//    amount of retries will be able to resolveLocked the txnID.
-	retryBudgetForMissingResult = uint32(1)
+	retryBudgetForMissingResult = uint32(2)
 
 	// retryBudgetForRPCFailure is the number of times the resolverQueue will
 	// retry resolving until giving up. This needs to be a finite number to handle