Merge #112082

112082: opt: move locking durability check back to optbuilder r=DrewKimball a=michae2

In #110945 I moved the locking durability check from optbuilder to execbuilder to allow a memo prepared under serializable isolation to execute under read committed isolation. As it turns out, however, memo staleness prevents this. Move the durability check back to optbuilder.

We have to keep the shared locking check in execbuilder to correctly throw errors if shared locking is used in a read-only transaction.

Informs: #100194

Release note: None

Co-authored-by: Michael Erickson <[email protected]>

pkg/sql/opt/exec/execbuilder/relational.go

@@ -2934,19 +2934,6 @@ func (b *Builder) buildLocking(locking opt.Locking) (opt.Locking, error) {
 				return opt.Locking{}, nil // early return; do not set b.ContainsNonDefaultKeyLocking
 			}
 		}
-		// Check if we can actually use guaranteed-durable locking here.
-		if locking.Durability == tree.LockDurabilityGuaranteed {
-			// Guaranteed-durable locking didn't exist prior to v23.2.
-			if !b.evalCtx.Settings.Version.IsActive(b.ctx, clusterversion.V23_2) ||
-				// Under serializable isolation we only use guaranteed-durable locks if
-				// enable_durable_locking_for_serializable is set. (Serializable
-				// isolation does not require locking for correctness, so by default we
-				// use best-effort locks for better performance.)
-				(b.evalCtx.TxnIsoLevel == isolation.Serializable &&
-					!b.evalCtx.SessionData().DurableLockingForSerializable) {
-				locking.Durability = tree.LockDurabilityBestEffort
-			}
-		}
 		b.ContainsNonDefaultKeyLocking = true
 	}
 	return locking, nil

pkg/sql/opt/memo/testdata/logprops/join

@@ -1955,15 +1955,15 @@ inner-join (hash)
  ├── interesting orderings: (+1) (+7) (-9,+10,+7)
  ├── scan fk
  │    ├── columns: k:1(int!null) v:2(int) r1:3(int!null) r2:4(int)
- │    ├── locking: for-update,skip-locked,durability-guaranteed
+ │    ├── locking: for-update,skip-locked
  │    ├── volatile
  │    ├── key: (1)
  │    ├── fd: (1)-->(2-4)
  │    ├── prune: (1-4)
  │    └── interesting orderings: (+1)
  ├── scan xysd
  │    ├── columns: x:7(int!null) y:8(int) s:9(string) d:10(decimal!null)
- │    ├── locking: for-update,skip-locked,durability-guaranteed
+ │    ├── locking: for-update,skip-locked
  │    ├── volatile
  │    ├── key: (7)
  │    ├── fd: (7)-->(8-10), (9,10)~~>(7,8)
@@ -1994,7 +1994,7 @@ inner-join (hash)
  │    └── unfiltered-cols: (1-6)
  ├── scan xysd
  │    ├── columns: x:7(int!null) y:8(int) s:9(string) d:10(decimal!null)
- │    ├── locking: for-share,skip-locked,durability-guaranteed
+ │    ├── locking: for-share,skip-locked
  │    ├── volatile
  │    ├── key: (7)
  │    ├── fd: (7)-->(8-10), (9,10)~~>(7,8)
@@ -2019,7 +2019,7 @@ inner-join (hash)
  ├── interesting orderings: (+1) (+7) (-9,+10,+7)
  ├── scan fk
  │    ├── columns: k:1(int!null) v:2(int) r1:3(int!null) r2:4(int)
- │    ├── locking: for-update,skip-locked,durability-guaranteed
+ │    ├── locking: for-update,skip-locked
  │    ├── volatile
  │    ├── key: (1)
  │    ├── fd: (1)-->(2-4)

pkg/sql/opt/memo/testdata/logprops/scan

@@ -285,7 +285,7 @@ project
  ├── interesting orderings: (+1) (-3,+4,+1)
  └── scan a
       ├── columns: x:1(int!null) y:2(int) s:3(string) d:4(decimal!null) crdb_internal_mvcc_timestamp:5(decimal) tableoid:6(oid)
-      ├── locking: for-update,durability-guaranteed
+      ├── locking: for-update
       ├── volatile
       ├── key: (1)
       ├── fd: (1)-->(2-6), (3,4)~~>(1,2,5,6)

pkg/sql/opt/optbuilder/select.go

@@ -14,6 +14,8 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation"
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
@@ -700,19 +702,25 @@ func (b *Builder) buildScan(
 	}
 	if locking.isSet() {
 		private.Locking = locking.get()
-		// Under weaker isolation levels we use fully-durable locks for SELECT FOR
-		// UPDATE statements, SELECT FOR SHARE statements, and constraint checks
-		// (e.g. FK checks), regardless of locking strength and wait policy. Unlike
-		// mutation statements, SELECT FOR UPDATE statements do not lay down
-		// intents, so we cannot rely on the durability of intents to guarantee
-		// exclusion until commit as we do for mutation statements. And unlike
-		// serializable isolation, weaker isolation levels do not perform read
-		// refreshing, so we cannot rely on read refreshing to guarantee exclusion.
-		//
-		// We set guaranteed durability here and then remove it in execbuilder if
-		// necessary, to allow for preparing and execution of statements in
-		// different isolation levels.
-		private.Locking.Durability = tree.LockDurabilityGuaranteed
+		if b.evalCtx.Settings.Version.IsActive(b.ctx, clusterversion.V23_2) &&
+			(b.evalCtx.TxnIsoLevel != isolation.Serializable ||
+				b.evalCtx.SessionData().DurableLockingForSerializable) {
+			// Under weaker isolation levels we use fully-durable locks for SELECT FOR
+			// UPDATE statements, SELECT FOR SHARE statements, and constraint checks
+			// (e.g. FK checks), regardless of locking strength and wait policy.
+			// Unlike mutation statements, SELECT FOR UPDATE statements do not lay
+			// down intents, so we cannot rely on the durability of intents to
+			// guarantee exclusion until commit as we do for mutation statements. And
+			// unlike serializable isolation, weaker isolation levels do not perform
+			// read refreshing, so we cannot rely on read refreshing to guarantee
+			// exclusion.
+			//
+			// Under serializable isolation we only use fully-durable locks if
+			// enable_durable_locking_for_serializable is set. (Serializable isolation
+			// does not require locking for correctness, so by default we use
+			// best-effort locks for better performance.)
+			private.Locking.Durability = tree.LockDurabilityGuaranteed
+		}
 		if private.Locking.WaitPolicy == tree.LockWaitSkipLocked && tab.FamilyCount() > 1 {
 			// TODO(rytaft): We may be able to support this if enough columns are
 			// pruned that only a single family is scanned.

pkg/sql/opt/optbuilder/testdata/fk-checks-insert

@@ -710,6 +710,6 @@ insert child
                 ├── scan parent
                 │    ├── columns: parent.p:8!null
                 │    ├── flags: disabled not visible index feature
-                │    └── locking: for-share,durability-guaranteed
+                │    └── locking: for-share
                 └── filters
                      └── p:7 = parent.p:8

pkg/sql/opt/optbuilder/testdata/fk-checks-update

@@ -600,7 +600,7 @@ update child
       │         ├── scan parent
       │         │    ├── columns: parent.p:13!null
       │         │    ├── flags: disabled not visible index feature
-      │         │    └── locking: for-share,durability-guaranteed
+      │         │    └── locking: for-share
       │         └── filters
       │              └── p:11 = parent.p:13
       ├── f-k-checks-item: grandchild(c) -> child(c)
@@ -670,7 +670,7 @@ update self
                 ├── scan self
                 │    ├── columns: x:11!null
                 │    ├── flags: disabled not visible index feature
-                │    └── locking: for-share,durability-guaranteed
+                │    └── locking: for-share
                 └── filters
                      └── y:10 = x:11