Merge #44350

44350: storage: revert "storage: rationalize server-side refreshes and fix bugs" r=andreimatei a=andreimatei

This reverts commit 1edb0d5.

Revert the main commit from #42969 (storage: rationalize server-side refreshes and fix bugs). 
It seems to be causing consistency problems, as seen in #43110.

Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>

pkg/kv/dist_sender_server_test.go

@@ -1731,7 +1731,7 @@ func TestTxnCoordSenderRetries(t *testing.T) {
 			// No retries, 1pc commit.
 		},
 		{
-			name: "require1PC commit with injected unknown serializable error",
+			name: "require1PC commit with injected possible replay error",
 			retryable: func(ctx context.Context, txn *client.Txn) error {
 				b := txn.NewBatch()
 				b.Put("a", "put")
@@ -2086,9 +2086,6 @@ func TestTxnCoordSenderRetries(t *testing.T) {
 			expFailure:    "unexpected value", // condition failed error when failing on tombstones
 		},
 		{
-			// This test sends a 1PC batch with Put+EndTxn.
-			// The Put gets a write too old error but, since there's no refresh spans,
-			// the commit succeeds.
 			name: "write too old with put in batch commit",
 			afterTxnStart: func(ctx context.Context, db *client.DB) error {
 				return db.Put(ctx, "a", "put")
@@ -2100,28 +2097,6 @@ func TestTxnCoordSenderRetries(t *testing.T) {
 			},
 			// No retries, 1pc commit.
 		},
-		{
-			// This test is like the previous one in that the commit batch succeeds at
-			// an updated timestamp, but this time the EndTxn puts the
-			// transaction in the STAGING state instead of COMMITTED because there had
-			// been previous write in a different batch. Like above, the commit is
-			// successful since there are no refresh spans.
-			name: "write too old in staging commit",
-			beforeTxnStart: func(ctx context.Context, db *client.DB) error {
-				return db.Put(ctx, "a", "orig")
-			},
-			afterTxnStart: func(ctx context.Context, db *client.DB) error {
-				return db.Put(ctx, "a", "put")
-			},
-			retryable: func(ctx context.Context, txn *client.Txn) error {
-				if err := txn.Put(ctx, "aother", "another put"); err != nil {
-					return err
-				}
-				b := txn.NewBatch()
-				b.Put("a", "final value")
-				return txn.CommitInBatch(ctx, b)
-			},
-		},
 		{
 			name: "write too old with cput in batch commit",
 			beforeTxnStart: func(ctx context.Context, db *client.DB) error {
@@ -2135,28 +2110,7 @@ func TestTxnCoordSenderRetries(t *testing.T) {
 				b.CPut("a", "cput", strToValue("put"))
 				return txn.CommitInBatch(ctx, b) // will be a 1PC, won't get auto retry
 			},
-			// No client-side retries, 1PC commit. On the server-side, the batch is
-			// evaluated twice: once at the original timestamp, where it gets a
-			// WriteTooOldError, and then once at the pushed timestamp. The
-			// server-side retry is enabled by the fact that there have not been any
-			// previous reads and so the transaction can commit at a pushed timestamp.
-		},
-		{
-			// This test is like the previous one, except the 1PC batch cannot commit
-			// at the updated timestamp.
-			name: "write too old with failed cput in batch commit",
-			beforeTxnStart: func(ctx context.Context, db *client.DB) error {
-				return db.Put(ctx, "a", "orig")
-			},
-			afterTxnStart: func(ctx context.Context, db *client.DB) error {
-				return db.Put(ctx, "a", "put")
-			},
-			retryable: func(ctx context.Context, txn *client.Txn) error {
-				b := txn.NewBatch()
-				b.CPut("a", "cput", strToValue("orig"))
-				return txn.CommitInBatch(ctx, b) // will be a 1PC, won't get auto retry
-			},
-			expFailure: "unexpected value", // The CPut cannot succeed.
+			// No retries, 1pc commit.
 		},
 		{
 			name: "multi-range batch with forwarded timestamp",
@@ -2286,8 +2240,7 @@ func TestTxnCoordSenderRetries(t *testing.T) {
 				b.Put("c", "put")
 				return txn.CommitInBatch(ctx, b)
 			},
-			// We expect the request to succeed after a server-side retry.
-			txnCoordRetry: false,
+			txnCoordRetry: true,
 		},
 		{
 			name: "multi-range batch with deferred write too old and failed cput",
@@ -2322,8 +2275,8 @@ func TestTxnCoordSenderRetries(t *testing.T) {
 				b.Put("c", "put")
 				return txn.CommitInBatch(ctx, b)
 			},
-			// We expect the request to succeed after a server-side retry.
-			txnCoordRetry: false,
+			// Expect a transaction coord retry, which should succeed.
+			txnCoordRetry: true,
 		},
 		{
 			name: "cput within uncertainty interval",

pkg/kv/txn_coord_sender_test.go

@@ -553,7 +553,6 @@ func TestTxnCoordSenderAddIntentOnError(t *testing.T) {
 }
 
 func assertTransactionRetryError(t *testing.T, e error) {
-	t.Helper()
 	if retErr, ok := e.(*roachpb.TransactionRetryWithProtoRefreshError); ok {
 		if !testutils.IsError(retErr, "TransactionRetryError") {
 			t.Fatalf("expected the cause to be TransactionRetryError, but got %s",

pkg/kv/txn_interceptor_span_refresher.go

@@ -170,19 +170,6 @@ func (sr *txnSpanRefresher) SendLocked(
 	// Send through wrapped lockedSender. Unlocks while sending then re-locks.
 	br, pErr, largestRefreshTS := sr.sendLockedWithRefreshAttempts(ctx, ba, maxAttempts)
 	if pErr != nil {
-		// The server sometimes "performs refreshes" - it updates the transaction's
-		// ReadTimestamp when the client said that there's nothing to refresh. In
-		// these cases, we need to update our refreshedTimestamp too. This is pretty
-		// inconsequential: the server only does this on an EndTxn. If the
-		// respective batch succeeds, then there won't be any more requests and so
-		// sr.refreshedTimestamp doesn't matter (that's why we don't handle the
-		// success case). However, the server can "refresh" and then return an
-		// error. In this case, the client will rollback and, if we don't update
-		// sr.refreshedTimestamp, then an assertion will fire about the rollback's
-		// timestamp being inconsistent.
-		if pErr.GetTxn() != nil {
-			sr.refreshedTimestamp.Forward(pErr.GetTxn().ReadTimestamp)
-		}
 		return nil, pErr
 	}
 

pkg/roachpb/api.go

@@ -107,6 +107,13 @@ func IsReadOnly(args Request) bool {
 	return (flags&isRead) != 0 && (flags&isWrite) == 0
 }
 
+// IsReadAndWrite returns true if the request both reads and writes
+// (such as conditional puts).
+func IsReadAndWrite(args Request) bool {
+	flags := args.flags()
+	return (flags&isRead) != 0 && (flags&isWrite) != 0
+}
+
 // IsTransactional returns true if the request may be part of a
 // transaction.
 func IsTransactional(args Request) bool {