kv: properly mark r/o txns as committed

Readonly txns have a special commit path. This path wasn't marking the
txn as committed correctly, which had a couple of consequences:
- db.Txn() would potentially try to commit again, in case the closure
already did it
- the txn.aborted metric was incremented

Fixes #31595

Release note: Fix a bug causing committed read-only txns to be counted
as aborted in the metrics.

pkg/kv/txn_coord_sender.go

@@ -566,6 +566,11 @@ func (tc *TxnCoordSender) commitReadOnlyTxnLocked(
 		return roachpb.NewError(
 			roachpb.NewTransactionStatusError("deadline exceeded before transaction finalization"))
 	}
+	tc.mu.txnState = txnFinalized
+	// Mark the transaction as committed so that, in case this commit is done by
+	// the closure passed to db.Txn()), db.Txn() doesn't attempt to commit again.
+	// Also so that the correct metric gets incremented.
+	tc.mu.txn.Status = roachpb.COMMITTED
 	tc.cleanupTxnLocked(ctx)
 	return nil
 }

pkg/kv/txn_coord_sender_test.go

@@ -1106,23 +1106,25 @@ func TestTxnCommit(t *testing.T) {
 	defer cleanupFn()
 	value := []byte("value")
 
-	// Test normal commit.
+	// Test a write txn commit.
 	if err := s.DB.Txn(context.TODO(), func(ctx context.Context, txn *client.Txn) error {
 		key := []byte("key-commit")
+		return txn.Put(ctx, key, value)
+	}); err != nil {
+		t.Fatal(err)
+	}
+	checkTxnMetrics(t, metrics, "commit txn", 1 /* commits */, 0 /* commits1PC */, 0, 0)
 
-		if err := txn.SetIsolation(enginepb.SNAPSHOT); err != nil {
-			return err
-		}
-
-		if err := txn.Put(ctx, key, value); err != nil {
-			return err
-		}
-
-		return txn.CommitOrCleanup(ctx)
+	// Test a read-only txn.
+	if err := s.DB.Txn(context.TODO(), func(ctx context.Context, txn *client.Txn) error {
+		key := []byte("key-commit")
+		_, err := txn.Get(ctx, key)
+		return err
 	}); err != nil {
 		t.Fatal(err)
 	}
-	checkTxnMetrics(t, metrics, "commit txn", 1, 0 /* not 1PC */, 0, 0)
+
+	checkTxnMetrics(t, metrics, "commit txn", 2 /* commits */, 0 /* commits1PC */, 0, 0)
 }
 
 // TestTxnOnePhaseCommit verifies that 1PC metric tracking works.

pkg/kv/txn_interceptor_metrics.go

@@ -109,6 +109,8 @@ func (m *txnMetrics) closeLocked() {
 		// transactions should be accounted.
 		m.metrics.Aborts.Inc(1)
 	case roachpb.COMMITTED:
+		// Note that successful read-only txn are also counted as committed, even
+		// though they never had a txn record.
 		m.metrics.Commits.Inc(1)
 	}
 }