kvcoord,concurrency: enable buffered writes by default

We would like to do this for as much of the 25.4 release cycle as
possible.

Buffered writes for weak isolation is still disabled as there is a long
tail of test failures to work through. A few of those test failures are
fixed in this PR.

Epic: none

Release note: None

Author: stevendanna

docs/generated/settings/settings-for-tenants.txt

@@ -100,7 +100,7 @@ kv.transaction.max_intents_bytes	integer	4194304	maximum number of bytes used to
 kv.transaction.max_refresh_spans_bytes	integer	4194304	maximum number of bytes used to track refresh spans in serializable transactions	application
 kv.transaction.randomized_anchor_key.enabled	boolean	false	dictates whether a transactions anchor key is randomized or not	application
 kv.transaction.reject_over_max_intents_budget.enabled	boolean	false	if set, transactions that exceed their lock tracking budget (kv.transaction.max_intents_bytes) are rejected instead of having their lock spans imprecisely compressed	application
-kv.transaction.write_buffering.enabled	boolean	false	if enabled, transactional writes are buffered on the client	application
+kv.transaction.write_buffering.enabled	boolean	true	if enabled, transactional writes are buffered on the client	application
 kv.transaction.write_buffering.max_buffer_size	byte size	4.0 MiB	if non-zero, defines that maximum size of the buffer that will be used to buffer transactional writes per-transaction	application
 kv.transaction.write_pipelining.locking_reads.enabled	boolean	true	if enabled, transactional locking reads are pipelined through Raft consensus	application
 kv.transaction.write_pipelining.ranged_writes.enabled	boolean	true	if enabled, transactional ranged writes are pipelined through Raft consensus	application

docs/generated/settings/settings.html

@@ -129,7 +129,7 @@
 <tr><td><div id="setting-kv-transaction-max-refresh-spans-bytes" class="anchored"><code>kv.transaction.max_refresh_spans_bytes</code></div></td><td>integer</td><td><code>4194304</code></td><td>maximum number of bytes used to track refresh spans in serializable transactions</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-kv-transaction-randomized-anchor-key-enabled" class="anchored"><code>kv.transaction.randomized_anchor_key.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>dictates whether a transactions anchor key is randomized or not</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-kv-transaction-reject-over-max-intents-budget-enabled" class="anchored"><code>kv.transaction.reject_over_max_intents_budget.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>if set, transactions that exceed their lock tracking budget (kv.transaction.max_intents_bytes) are rejected instead of having their lock spans imprecisely compressed</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
-<tr><td><div id="setting-kv-transaction-write-buffering-enabled" class="anchored"><code>kv.transaction.write_buffering.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>if enabled, transactional writes are buffered on the client</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-kv-transaction-write-buffering-enabled" class="anchored"><code>kv.transaction.write_buffering.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>if enabled, transactional writes are buffered on the client</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-kv-transaction-write-buffering-max-buffer-size" class="anchored"><code>kv.transaction.write_buffering.max_buffer_size</code></div></td><td>byte size</td><td><code>4.0 MiB</code></td><td>if non-zero, defines that maximum size of the buffer that will be used to buffer transactional writes per-transaction</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-kv-transaction-write-pipelining-locking-reads-enabled" class="anchored"><code>kv.transaction.write_pipelining.locking_reads.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>if enabled, transactional locking reads are pipelined through Raft consensus</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-kv-transaction-write-pipelining-ranged-writes-enabled" class="anchored"><code>kv.transaction.write_pipelining.ranged_writes.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>if enabled, transactional ranged writes are pipelined through Raft consensus</td><td>Serverless/Dedicated/Self-Hosted</td></tr>

pkg/ccl/logictestccl/testdata/logic_test/read_committed

@@ -515,6 +515,11 @@ GRANT ALL ON t TO testuser2;
 
 user testuser
 
+# The following transaction looks at the lock table, which makes it sensitive to whether or not
+# replicated locks are buffered.
+statement ok
+SET kv_transaction_buffered_writes_enabled=false
+
 query I
 BEGIN ISOLATION LEVEL READ COMMITTED;
 SELECT * FROM t WHERE a = 1 FOR SHARE;
@@ -532,6 +537,9 @@ database_name  schema_name  table_name  lock_key_pretty  lock_strength  durabili
 # 1: Shared
 # 2: None
 
+statement ok
+SET kv_transaction_buffered_writes_enabled=false
+
 query I
 BEGIN ISOLATION LEVEL READ COMMITTED;
 SELECT * FROM t FOR UPDATE SKIP LOCKED
@@ -544,23 +552,35 @@ user testuser2
 # 1: Shared
 # 2: Exclusive
 
+statement ok
+SET kv_transaction_buffered_writes_enabled=false
+
 query I
 BEGIN ISOLATION LEVEL READ COMMITTED;
 SELECT * FROM t FOR SHARE SKIP LOCKED;
 COMMIT;
 ----
 1
 
+statement ok
+RESET kv_transaction_buffered_writes_enabled
+
 user root
 
 statement ok
 COMMIT
 
+statement ok
+RESET kv_transaction_buffered_writes_enabled
+
 user testuser
 
 statement ok
 COMMIT
 
+statement ok
+RESET kv_transaction_buffered_writes_enabled
+
 subtest end
 
 subtest regression_130661

pkg/ccl/testccl/sqlccl/BUILD.bazel

@@ -34,6 +34,7 @@ go_test(
         "//pkg/kv/kvpb",
         "//pkg/kv/kvserver",
         "//pkg/kv/kvserver/concurrency/isolation",
+        "//pkg/kv/kvserver/concurrency/lock",
         "//pkg/kv/kvserver/protectedts/ptpb",
         "//pkg/multitenant/mtinfopb",
         "//pkg/roachpb",

pkg/ccl/testccl/sqlccl/read_committed_test.go

@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
@@ -153,14 +154,34 @@ func TestReadCommittedReadTimestampNotSteppedOnCommit(t *testing.T) {
 	// Keep track of the read timestamps of the read committed transaction during
 	// each KV operation.
 	var txnReadTimestamps []hlc.Timestamp
+	var txnShouldParallelCommit bool
 	filterFunc := func(ctx context.Context, ba *kvpb.BatchRequest, _ *kvpb.BatchResponse) *kvpb.Error {
 		if ba.Txn == nil || ba.Txn.IsoLevel != isolation.ReadCommitted {
 			return nil
 		}
-		req := ba.Requests[0]
-		method := req.GetInner().Method()
-		if method == kvpb.ConditionalPut || (method == kvpb.EndTxn && req.GetEndTxn().IsParallelCommit()) {
+		req := ba.Requests[len(ba.Requests)-1]
+
+		var recordRead bool
+		switch req.GetInner().Method() {
+		case kvpb.ConditionalPut:
+			recordRead = true
+			txnShouldParallelCommit = true
+		case kvpb.Get:
+			recordRead = req.GetGet().KeyLockingStrength == lock.Exclusive
+		case kvpb.EndTxn:
+			if txnShouldParallelCommit {
+				recordRead = req.GetEndTxn().IsParallelCommit()
+			} else {
+				recordRead = true
+			}
+		default:
+			recordRead = false
+		}
+		if recordRead {
+			t.Logf("recording timestamp for %s", req)
 			txnReadTimestamps = append(txnReadTimestamps, ba.Txn.ReadTimestamp)
+		} else {
+			t.Logf("not recording timestamp for %s", req)
 		}
 		return nil
 	}

pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer.go

@@ -40,7 +40,7 @@ var BufferedWritesEnabled = settings.RegisterBoolSetting(
 	settings.ApplicationLevel,
 	"kv.transaction.write_buffering.enabled",
 	"if enabled, transactional writes are buffered on the client",
-	metamorphic.ConstantWithTestBool("kv.transaction.write_buffering.enabled", false /* defaultValue */),
+	metamorphic.ConstantWithTestBool("kv.transaction.write_buffering.enabled", true /* defaultValue */),
 	settings.WithPublic,
 )
 

pkg/kv/kvserver/concurrency/concurrency_manager.go

@@ -24,7 +24,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
-	"github.com/cockroachdb/cockroach/pkg/util/buildutil"
 	"github.com/cockroachdb/cockroach/pkg/util/debugutil"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -132,13 +131,7 @@ var UnreplicatedLockReliabilityLeaseTransfer = settings.RegisterBoolSetting(
 	settings.SystemOnly,
 	"kv.lock_table.unreplicated_lock_reliability.lease_transfer.enabled",
 	"whether the replica should attempt to keep unreplicated locks during lease transfers",
-	metamorphic.ConstantWithTestBool("kv.lock_table.unreplicated_lock_reliability.lease_transfer.enabled", false),
-	settings.WithValidateBool(func(_ *settings.Values, enabled bool) error {
-		if enabled && !buildutil.CrdbTestBuild {
-			return errors.Newf("kv.lock_table.unreplicated_lock_reliability.lease_transfer.enabled is not supported in production builds")
-		}
-		return nil
-	}),
+	metamorphic.ConstantWithTestBool("kv.lock_table.unreplicated_lock_reliability.lease_transfer.enabled", true),
 )
 
 // UnreplicatedLockReliabilityMerge controls whether the replica will attempt to
@@ -147,13 +140,7 @@ var UnreplicatedLockReliabilityMerge = settings.RegisterBoolSetting(
 	settings.SystemOnly,
 	"kv.lock_table.unreplicated_lock_reliability.merge.enabled",
 	"whether the replica should attempt to keep unreplicated locks during range merges",
-	metamorphic.ConstantWithTestBool("kv.lock_table.unreplicated_lock_reliability.merge.enabled", false),
-	settings.WithValidateBool(func(_ *settings.Values, enabled bool) error {
-		if enabled && !buildutil.CrdbTestBuild {
-			return errors.Newf("kv.lock_table.unreplicated_lock_reliability.merge.enabled is not supported in production builds")
-		}
-		return nil
-	}),
+	metamorphic.ConstantWithTestBool("kv.lock_table.unreplicated_lock_reliability.merge.enabled", true),
 )
 
 var MaxLockFlushSize = settings.RegisterByteSizeSetting(