sql: only use side-effect splitter if locking durability = guaranteed

The span splitter has an optimization to skip reading column families
whose columns are contained entirely in the index key. In #116170 we
added a flag to the span splitter to ignore this optimization when the
read has a side-effect (i.e. it's a locking read).

The intention of #116170 was to ensure that SELECT FOR UPDATE statements
under Read Committed isolation lock all requested column families. But
the span splitter flag also applies to the implicit for-update locking
added to mutation statements. This was not intentional. The implicit
for-update locking added to mutation statements is not needed for
correctness, but is supposed to be a lightweight optimization to prevent
retries of racing write-write conflicts. The implicit for-update locking
is already not durable. It's fine if it does not lock every requested
column family.

This commit changes the decision about whether to use the span splitter
optimization to depend on the locking durability, rather than the
isolation level. This extends the meaning of locking durability
slightly, but I think it fits within the semantics of best effort = the
requested lock might not actually be held.

This should fix the YCSB regression under Read Committed isolation.

Fixes: #143138

Release note: None

Author: michae2

pkg/ccl/logictestccl/testdata/logic_test/read_committed

@@ -594,3 +594,40 @@ statement ok
 ROLLBACK;
 
 subtest end
+
+subtest regression_143138
+
+statement ok
+CREATE TABLE usertable (
+  ycsb_key VARCHAR(255) NOT NULL,
+  field0 STRING NOT NULL,
+  field1 STRING NOT NULL,
+  CONSTRAINT usertable_pkey PRIMARY KEY (ycsb_key ASC),
+  FAMILY fam_0_ycsb_key (ycsb_key),
+  FAMILY fam_1_field0 (field0),
+  FAMILY fam_2_field1 (field1)
+)
+
+statement ok
+SET plan_cache_mode = force_generic_plan
+
+statement ok
+PREPARE p AS UPDATE usertable SET field1 = $2:::STRING WHERE ycsb_key = $1:::STRING;
+
+query T kvtrace
+EXECUTE p ('foo', 'bar')
+----
+Scan /Table/119/1/"foo"/2/1 lock Exclusive (Block, Unreplicated)
+
+statement ok
+BEGIN ISOLATION LEVEL READ COMMITTED;
+
+query T kvtrace
+EXECUTE p ('foo', 'bar')
+----
+Scan /Table/119/1/"foo"/2/1 lock Exclusive (Block, Unreplicated)
+
+statement ok
+ROLLBACK
+
+subtest end

pkg/sql/distsql_physical_planner.go

@@ -21,7 +21,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvclient"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord"
-	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/rpc"
 	"github.com/cockroachdb/cockroach/pkg/rpc/nodedialer"
@@ -3460,7 +3459,7 @@ func (dsp *DistSQLPlanner) planLookupJoin(
 
 	var splitter span.Splitter
 	if joinReaderSpec.LockingStrength != descpb.ScanLockingStrength_FOR_NONE &&
-		planCtx.ExtendedEvalCtx.TxnIsoLevel != isolation.Serializable {
+		joinReaderSpec.LockingDurability == descpb.ScanLockingDurability_GUARANTEED {
 		splitter = span.MakeSplitterForSideEffect(planInfo.fetch.desc, planInfo.fetch.index, fetchOrdinals)
 	} else {
 		splitter = span.MakeSplitter(planInfo.fetch.desc, planInfo.fetch.index, fetchOrdinals)