sql: use the correct locking strength for FOR SHARE clause

Previously, FOR SHARE and FOR KEY SHARE would use non-locking KV scans.
Now that the lock table supports shared locks, we can use lock.Shared as
the locking strength for KV scans. This patch does that, and in doing
so, wires up SHARED locks end to end.

By default, we turn of this functionality for serializable transactions.
Instead, it's gated behind a session setting called
`enable_shared_locking_for_serializable`.

Informs cockroachdb#91545

Release note (sql change): Shared locks are a thing.

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

@@ -248,6 +248,7 @@ sql.insights.anomaly_detection.memory_limit	byte size	1.0 MiB	the maximum amount
 sql.insights.execution_insights_capacity	integer	1000	the size of the per-node store of execution insights	tenant-rw
 sql.insights.high_retry_count.threshold	integer	10	the number of retries a slow statement must have undergone for its high retry count to be highlighted as a potential problem	tenant-rw
 sql.insights.latency_threshold	duration	100ms	amount of time after which an executing statement is considered slow. Use 0 to disable.	tenant-rw
+sql.locking.enable_shared_locks	boolean	false	enable shared locking strength when using FOR SHARE or FOR KEY SHARE modifiers	tenant-ro
 sql.log.slow_query.experimental_full_table_scans.enabled	boolean	false	when set to true, statements that perform a full table/index scan will be logged to the slow query log even if they do not meet the latency threshold. Must have the slow query log enabled for this setting to have any effect.	tenant-rw
 sql.log.slow_query.internal_queries.enabled	boolean	false	when set to true, internal queries which exceed the slow query log threshold are logged to a separate log. Must have the slow query log enabled for this setting to have any effect.	tenant-rw
 sql.log.slow_query.latency_threshold	duration	0s	when set to non-zero, log statements whose service latency exceeds the threshold to a secondary logger on each node	tenant-rw

docs/generated/settings/settings.html

@@ -195,6 +195,7 @@
 <tr><td><div id="setting-sql-insights-execution-insights-capacity" class="anchored"><code>sql.insights.execution_insights_capacity</code></div></td><td>integer</td><td><code>1000</code></td><td>the size of the per-node store of execution insights</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-sql-insights-high-retry-count-threshold" class="anchored"><code>sql.insights.high_retry_count.threshold</code></div></td><td>integer</td><td><code>10</code></td><td>the number of retries a slow statement must have undergone for its high retry count to be highlighted as a potential problem</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-sql-insights-latency-threshold" class="anchored"><code>sql.insights.latency_threshold</code></div></td><td>duration</td><td><code>100ms</code></td><td>amount of time after which an executing statement is considered slow. Use 0 to disable.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-sql-locking-enable-shared-locks" class="anchored"><code>sql.locking.enable_shared_locks</code></div></td><td>boolean</td><td><code>false</code></td><td>enable shared locking strength when using FOR SHARE or FOR KEY SHARE modifiers</td><td>Serverless/Dedicated/Self-Hosted (read-only)</td></tr>
 <tr><td><div id="setting-sql-log-slow-query-experimental-full-table-scans-enabled" class="anchored"><code>sql.log.slow_query.experimental_full_table_scans.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>when set to true, statements that perform a full table/index scan will be logged to the slow query log even if they do not meet the latency threshold. Must have the slow query log enabled for this setting to have any effect.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-sql-log-slow-query-internal-queries-enabled" class="anchored"><code>sql.log.slow_query.internal_queries.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>when set to true, internal queries which exceed the slow query log threshold are logged to a separate log. Must have the slow query log enabled for this setting to have any effect.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-sql-log-slow-query-latency-threshold" class="anchored"><code>sql.log.slow_query.latency_threshold</code></div></td><td>duration</td><td><code>0s</code></td><td>when set to non-zero, log statements whose service latency exceeds the threshold to a secondary logger on each node</td><td>Serverless/Dedicated/Self-Hosted</td></tr>

pkg/sql/BUILD.bazel

@@ -579,6 +579,7 @@ go_library(
         "//pkg/util/tsearch",
         "//pkg/util/uint128",
         "//pkg/util/uuid",
+        "@com_github_cockroachdb_apd//:apd",
         "@com_github_cockroachdb_apd_v3//:apd",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_cockroachdb_errors//hintdetail",

pkg/sql/exec_util.go

@@ -3638,6 +3638,10 @@ func (m *sessionDataMutator) SetDurableLockingForSerializable(val bool) {
 	m.data.DurableLockingForSerializable = val
 }
 
+func (m *sessionDataMutator) SetSharedLockingForSerializable(val bool) {
+	m.data.SharedLockingForSerializable = val
+}
+
 // Utility functions related to scrubbing sensitive information on SQL Stats.
 
 // quantizeCounts ensures that the Count field in the

pkg/sql/logictest/testdata/logic_test/information_schema

@@ -5346,6 +5346,7 @@ enable_insert_fast_path                                    on
 enable_multiple_modifications_of_table                     off
 enable_multiregion_placement_policy                        off
 enable_seqscan                                             on
+enable_shared_locking_for_serializable                     off
 enable_super_regions                                       off
 enable_zigzag_join                                         off
 enforce_home_region                                        off

pkg/sql/logictest/testdata/logic_test/pg_catalog

@@ -2788,6 +2788,7 @@ enable_insert_fast_path                                    on                  N
 enable_multiple_modifications_of_table                     off                 NULL      NULL        NULL        string
 enable_multiregion_placement_policy                        off                 NULL      NULL        NULL        string
 enable_seqscan                                             on                  NULL      NULL        NULL        string
+enable_shared_locking_for_serializable                     off                 NULL      NULL        NULL        string
 enable_super_regions                                       off                 NULL      NULL        NULL        string
 enable_zigzag_join                                         off                 NULL      NULL        NULL        string
 enforce_home_region                                        off                 NULL      NULL        NULL        string
@@ -2949,6 +2950,7 @@ enable_insert_fast_path                                    on                  N
 enable_multiple_modifications_of_table                     off                 NULL  user     NULL      off                 off
 enable_multiregion_placement_policy                        off                 NULL  user     NULL      off                 off
 enable_seqscan                                             on                  NULL  user     NULL      on                  on
+enable_shared_locking_for_serializable                     off                 NULL  user     NULL      off                 off
 enable_super_regions                                       off                 NULL  user     NULL      off                 off
 enable_zigzag_join                                         off                 NULL  user     NULL      off                 off
 enforce_home_region                                        off                 NULL  user     NULL      off                 off
@@ -3107,6 +3109,7 @@ enable_insert_fast_path                                    NULL    NULL     NULL
 enable_multiple_modifications_of_table                     NULL    NULL     NULL     NULL        NULL
 enable_multiregion_placement_policy                        NULL    NULL     NULL     NULL        NULL
 enable_seqscan                                             NULL    NULL     NULL     NULL        NULL
+enable_shared_locking_for_serializable                     NULL    NULL     NULL     NULL        NULL
 enable_super_regions                                       NULL    NULL     NULL     NULL        NULL
 enable_zigzag_join                                         NULL    NULL     NULL     NULL        NULL
 enforce_home_region                                        NULL    NULL     NULL     NULL        NULL

pkg/sql/logictest/testdata/logic_test/select_for_share

@@ -0,0 +1,117 @@
+# LogicTest: !local-mixed-22.2-23.1
+
+statement ok
+CREATE TABLE t(a INT PRIMARY KEY);
+INSERT INTO t VALUES(1);
+GRANT ALL ON t TO testuser;
+CREATE USER testuser2 WITH VIEWACTIVITY;
+GRANT SYSTEM MODIFYCLUSTERSETTING TO testuser;
+GRANT ALL ON t TO testuser2;
+
+user testuser
+
+statement ok
+SET enable_shared_locking_for_serializable = true;
+
+statement ok
+BEGIN
+
+query I
+SELECT * FROM t WHERE a = 1 FOR SHARE;
+----
+1
+
+# Start another transaction to show multiple transactions can acquire SHARED
+# locks at the same time.
+
+user root
+
+statement ok
+SET enable_shared_locking_for_serializable = true;
+
+statement ok
+BEGIN
+
+query I
+SELECT * FROM t  WHERE a = 1 FOR SHARE;
+----
+1
+
+user testuser2
+
+statement async writeReq count 1
+UPDATE t SET a = 2 WHERE a = 1
+
+# TODO(arul): Until https://github.com/cockroachdb/cockroach/issues/107766 is
+# addressed, we'll incorrectly report shared locks as having "Exclusive" lock
+# strength; however, having this query in here is useful to make sure there are
+# two locks on our key and setting the cluster setting above actually did
+# something; otherwise, had we used non-locking reads, we'd have failed here.
+query TTTTTTTBB colnames,retry,rowsort
+SELECT database_name, schema_name, table_name, lock_key_pretty, lock_strength, durability, isolation_level, granted, contended FROM crdb_internal.cluster_locks
+----
+database_name  schema_name  table_name  lock_key_pretty   lock_strength  durability    isolation_level  granted  contended
+test           public       t           /Table/106/1/1/0  Exclusive      Unreplicated  SERIALIZABLE     true     true
+test           public       t           /Table/106/1/1/0  Exclusive      Unreplicated  SERIALIZABLE     false    true
+
+# Commit the first transaction and rollback the second.
+
+user testuser
+
+statement ok
+COMMIT
+
+user root
+
+statement ok
+ROLLBACK
+
+user testuser2
+
+# Now that both the transactions that issued shared lock reads have been
+# finalized, the write should be able to proceed.
+
+awaitstatement writeReq
+
+query I
+SELECT * FROM t;
+----
+2
+
+# ------------------------------------------------------------------------------
+# Tests to ensure the enable_shared_locking_for_serializable session variable
+# works as expected.
+# -----------------------------------------------------------------------------
+
+user testuser
+
+statement ok
+SET enable_shared_locking_for_serializable = false
+
+statement ok
+BEGIN ISOLATION LEVEL SERIALIZABLE
+
+query I
+SELECT * FROM t WHERE a = 2 FOR SHARE
+----
+2
+
+user testuser2
+
+query TTTTTTTBB colnames,retry,rowsort
+SELECT database_name, schema_name, table_name, lock_key_pretty, lock_strength, durability, isolation_level, granted, contended FROM crdb_internal.cluster_locks
+----
+database_name  schema_name  table_name  lock_key_pretty   lock_strength  durability    isolation_level  granted  contended
+
+user testuser
+
+statement ok
+COMMIT
+
+# TODO(arul): Add a test to show that the session setting doesn't apply to read
+# committed transactions. We currently can't issue SELECT FOR SHARE statmenets
+# in read committed transactions because durable locking hasn't been
+# fully hooked up.
+
+
+

pkg/sql/logictest/testdata/logic_test/show_source

@@ -70,6 +70,7 @@ enable_insert_fast_path                                    on
 enable_multiple_modifications_of_table                     off
 enable_multiregion_placement_policy                        off
 enable_seqscan                                             on
+enable_shared_locking_for_serializable                     off
 enable_super_regions                                       off
 enable_zigzag_join                                         off
 enforce_home_region                                        off

pkg/sql/opt/exec/execbuilder/testdata/select_for_update

@@ -42,6 +42,26 @@ vectorized: true
   spans: FULL SCAN
   locking strength: for no key update
 
+# By default, SELECT FOR SHARE doesn't acquire any locks.
+
+query T
+EXPLAIN (VERBOSE) SELECT * FROM t FOR SHARE
+----
+distribution: local
+vectorized: true
+·
+• scan
+  columns: (a, b)
+  estimated row count: 1,000 (missing stats)
+  table: t@t_pkey
+  spans: FULL SCAN
+
+# However, if the session setting is configured to do so, SELECT FOR SHARE will
+# acquire shared locks.
+
+statement ok
+SET enable_shared_locking_for_serializable = true
+
 query T
 EXPLAIN (VERBOSE) SELECT * FROM t FOR SHARE
 ----

pkg/sql/opt/optbuilder/BUILD.bazel

@@ -50,6 +50,7 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder",
     visibility = ["//visibility:public"],
     deps = [
+        "//pkg/clusterversion",
         "//pkg/kv/kvserver/concurrency/isolation",
         "//pkg/server/telemetry",
         "//pkg/settings",

pkg/sql/opt/optbuilder/select.go

@@ -14,6 +14,7 @@ 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"
@@ -697,6 +698,7 @@ func (b *Builder) buildScan(
 	}
 	if locking.isSet() {
 		private.Locking = locking.get()
+		// TODO(arul): This needs a cluster version check.
 		if b.evalCtx.TxnIsoLevel != isolation.Serializable ||
 			b.evalCtx.SessionData().DurableLockingForSerializable {
 			// Under weaker isolation levels we use fully-durable locks for SELECT FOR
@@ -715,6 +717,25 @@ func (b *Builder) buildScan(
 			// best-effort locks for better performance.)
 			private.Locking.Durability = tree.LockDurabilityGuaranteed
 		}
+		// Check if we can actually use shared locks here , or we need to use
+		// non-locking reads instead.
+		if private.Locking.Strength == tree.ForShare || private.Locking.Strength == tree.ForKeyShare {
+			// Shared locks weren't a thing prior to v23.2, so we must use non-locking
+			// reads.
+			if !b.evalCtx.Settings.Version.IsActive(context.TODO(), clusterversion.V23_2) ||
+				// And in >= v23.2, their locking behavior for serializable transactions
+				// is dictated by session setting.
+				(b.evalCtx.TxnIsoLevel == isolation.Serializable &&
+					!b.evalCtx.SessionData().SharedLockingForSerializable) {
+				private.Locking.Strength = tree.ForNone
+				// Key locking strength and durability go hand in hand. If we're
+				// overriding what should have been a shared locking read to
+				// non-locking, we also need to reset the lock durability; otherwise, KV
+				// won't be too happy if we start asking for nonsensical "replicated
+				// non-locking locks".
+				private.Locking.Durability = tree.LockDurabilityBestEffort
+			}
+		}
 		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/row/locking.go

@@ -27,9 +27,7 @@ func GetKeyLockingStrength(lockStrength descpb.ScanLockingStrength) lock.Strengt
 		// Promote to FOR_SHARE.
 		fallthrough
 	case descpb.ScanLockingStrength_FOR_SHARE:
-		// We currently perform no per-key locking when FOR_SHARE is used
-		// because Shared locks have not yet been implemented.
-		return lock.None
+		return lock.Shared
 
 	case descpb.ScanLockingStrength_FOR_NO_KEY_UPDATE:
 		// Promote to FOR_UPDATE.

pkg/sql/sessiondatapb/local_only_session_data.proto

@@ -420,9 +420,17 @@ message LocalOnlySessionData {
   // DurableLockingForSerializable is true if we should use durable locking for
   // SELECT FOR UPDATE statements, SELECT FOR SHARE statements, and constraint
   // checks under serializable isolation. (Serializable isolation does not
-  // require locking for correctness, so by default we use best-effor locks for
+  // require locking for correctness, so by default we use best-effort locks for
   // better performance.) Weaker isolation levels always use durable locking.
   bool durable_locking_for_serializable = 109;
+  // SharedLockingForSerializable, if set to true, means SELECT FOR  SHARE and
+  // SELECT FOR KEY SHARE statements issued by transactions that run with
+  // Serializable isolation will acquiring shared locks; otherwise, they'll
+  // perform non-locking reads.
+  //
+  // Weaker isolation levels always acquire shared locks for SELECT FOR SHARE
+  // and SELECT FOR KEY SHARE statements, regardless of this session setting.
+  bool shared_locking_for_serializable = 112;
   // MaxRetriesForReadCommitted indicates the maximum number of
   // automatic retries to perform for statements in explicit READ COMMITTED
   // transactions that see a transaction retry error.