Merge #87350 #87773 #87803 #87821 #87827

87350: xform: avoid locality-optimized scans which must always read remote rows r=msirek a=msirek

Previously, for tables with old-style partitioning, which don't use the
new multiregion abstractions, there were no guardrails in place to
prevent 2 cases where locality-optimized scan must always read ranges in
remote regions:

1. When a scan with no hard limit has a non-unique index constraint
   (could return more than one row per matched index key, not including
   the partitioning key column)
2. When the max cardinality of a constrained scan is less than the hard
   limit placed on the scan via a LIMIT clause

This was inadequate because locality-optimized scan is usually slower
than distributed scans when reading from remote regions is required. If
we can statically determine reading from remote regions is required,
locality-optimized scan should not even be costed and considered by the
optimizer. Multiregion tables, such as REGIONAL BY ROW tables, don't
encounter this issue because the internal `crdb_region` partitioning
column is not part of the UNIQUE constraint in that case, for example:
```
CREATE TABLE regional_by_row_table (
  col1 int,
  col2 bool NOT NULL,
  UNIQUE INDEX idx(col1) -- crdb_region is implicitly the 1st idx col
) LOCALITY REGIONAL BY ROW;

SELECT * FROM regional_by_row_table WHERE col1 = 1;
```
In the above, we could use LOS and split this into a local scan:
`SELECT * FROM regional_by_row_table WHERE crdb_region = 'us' AND col1 = 1;`
... and remote scans:
```
SELECT * FROM regional_by_row_table WHERE crdb_region IN ('ca', 'ap')
          AND col1 = 1;
```
The max cardinality of the local scan is 1, and the max cardinality of
the original scan is 1, so we know it's possible to fulfill the request
solely with the local scan.

To address this, this patch avoids planning locality-optimized scan for
the two cases listed at the top of the description. The first case is
detected by the local scan of the UNION ALL having a lower max
cardinality than the max cardinality including all constraint spans
(for example, given a pair of columns (part_col, col1), if col1 is a
unique key, then max_cardinality(col1) will equal 
max_cardinality(part_col, col1). The second case is detected by a
direct comparison of the hard limit with the max cardinality of the
local scan.

Release note (bug fix): This patch fixes a misused query optimization
involving tables with one or more PARTITION BY clauses and partition
zone constraints which assign region locality to those partitions.
In some cases the optimizer picks a `locality-optimized search` query
plan which is not truly locality-optimized, and has higher latency than
competing query plans which use distributed scan. Locality-optimized
search is now avoided in cases which are known not to benefit from this
optimization.

Release justification: Low risk fix for suboptimal locality-optimized scan

87773: scheduledlogging: various fixes to index stat collection r=xinhaoz a=knz

As I was reviewing #87525, I noticed that the actual test cases were missing.
Then, as I tried to fix that, I discovered a couple of other shortcomings. This PR fixes them.

Fixes #87771.
Informs #87772.



87803: ui: show app information on SQL Activity r=maryliag a=maryliag

Add Application Name to:
- Statement Overview
<img width="670" alt="Screen Shot 2022-09-11 at 10 24 26 PM" src="https://user-images.githubusercontent.com/1017486/189563090-f60eeb52-07ba-47fd-8560-223ccfa091c4.png">

- Transaction Overview
<img width="699" alt="Screen Shot 2022-09-11 at 10 24 41 PM" src="https://user-images.githubusercontent.com/1017486/189563108-2fd0599c-410b-42d6-affb-ef885908adab.png">


- Transaction Details
<img width="1593" alt="Screen Shot 2022-09-11 at 10 24 52 PM" src="https://user-images.githubusercontent.com/1017486/189563118-ee1360cc-28a2-41cd-afbf-c0924b0c9fd8.png">


Fixes #87782
Partially Addresses #85229

Rename from "App" to "Application Name" on
Statement Details.
<img width="787" alt="Screen Shot 2022-09-11 at 10 25 08 PM" src="https://user-images.githubusercontent.com/1017486/189563136-a9d041b3-a667-409f-aa94-766ec23dafdd.png">


Release note (ui change): Add Application Name to
Statement overview, Transaction Overview (and their respective column selectors), Transaction Details. Update label from "App" to "Application Name" on Statement Details page.

87821: roachtest: increase slowness threshold in streamer subtest r=yuzefovich a=yuzefovich

This commit bumps up the slowness threshold for the `streamer` subtest from 1.25 to 1.5 in order to eliminate rare flakes on Q1 (which doesn't use the streamer at all). 1.5 threshold should still be sufficient as a sanity check (that the streamer doesn't become extremely slow).

Fixes: #87791.

Release note: None

87827: sql/schemachanger/rel,testutils/lint/passes/errcmp: add nolint support, use r=ajwerner a=ajwerner

This error check was very expensive and was showing up meaningfully in profiles.

Release note: None

Co-authored-by: Mark Sirek <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Marylia Gutierrez <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>

pkg/BUILD.bazel

@@ -500,6 +500,7 @@ ALL_TESTS = [
     "//pkg/testutils/docker:docker_test",
     "//pkg/testutils/floatcmp:floatcmp_test",
     "//pkg/testutils/keysutils:keysutils_test",
+    "//pkg/testutils/lint/passes/errcmp:errcmp_test",
     "//pkg/testutils/lint/passes/errwrap:errwrap_test",
     "//pkg/testutils/lint/passes/fmtsafe:fmtsafe_test",
     "//pkg/testutils/lint/passes/forbiddenmethod:forbiddenmethod_test",
@@ -1796,6 +1797,7 @@ GO_TARGETS = [
     "//pkg/testutils/lint/passes/descriptormarshal:descriptormarshal",
     "//pkg/testutils/lint/passes/errcheck:errcheck",
     "//pkg/testutils/lint/passes/errcmp:errcmp",
+    "//pkg/testutils/lint/passes/errcmp:errcmp_test",
     "//pkg/testutils/lint/passes/errwrap:errwrap",
     "//pkg/testutils/lint/passes/errwrap:errwrap_test",
     "//pkg/testutils/lint/passes/fmtsafe:fmtsafe",

pkg/ccl/logictestccl/testdata/logic_test/multi_region_locality_optimized_search_query_behavior

@@ -0,0 +1,151 @@
+# tenant-cluster-setting-override-opt: allow-multi-region-abstractions-for-secondary-tenants
+# LogicTest: multiregion-9node-3region-3azs
+# TODO(#75864): enable multiregion-9node-3region-3azs-tenant.
+
+# Set the closed timestamp interval to be short to shorten the amount of time
+# we need to wait for the system config to propagate.
+statement ok
+SET CLUSTER SETTING kv.closed_timestamp.side_transport_interval = '10ms';
+
+statement ok
+SET CLUSTER SETTING kv.closed_timestamp.target_duration = '10ms';
+
+############################################
+# Regression tests for support issue #1780 #
+############################################
+statement ok
+CREATE DATABASE test_db
+
+statement ok
+USE test_db
+
+statement ok
+SET experimental_enable_unique_without_index_constraints = true
+
+statement ok
+CREATE TABLE users (
+        account_id UUID NOT NULL,
+        home_region STRING NOT NULL,
+        address STRING NOT NULL,
+        CONSTRAINT users_pkey PRIMARY KEY (home_region ASC, account_id ASC),
+        UNIQUE INDEX home_region_and_address (home_region ASC, address ASC) PARTITION BY LIST (home_region) (
+                PARTITION ca VALUES IN (('ca')),
+                PARTITION us VALUES IN (('us')),
+                PARTITION ap VALUES IN (('ap'))
+        ),
+        CONSTRAINT check_home_region CHECK (home_region IN ('ap':::STRING, 'us':::STRING, 'ca':::STRING))
+) PARTITION BY LIST (home_region) (
+        PARTITION ca VALUES IN (('ca')),
+        PARTITION us VALUES IN (('us')),
+        PARTITION ap VALUES IN (('ap'))
+);
+
+statement ok
+ALTER PARTITION "ca" OF INDEX users@* CONFIGURE ZONE USING
+  constraints = '[+region=ca-central-1]';
+
+statement ok
+ALTER PARTITION "us" OF INDEX users@* CONFIGURE ZONE USING
+  constraints = '[+region=us-east-1]';
+
+statement ok
+ALTER PARTITION "ap" OF INDEX users@* CONFIGURE ZONE USING
+  constraints = '[+region=ap-southeast-2]';
+
+# Locality-optimized scan should not be used when rows from other regions
+# might exist and need to be returned.
+query T retry
+EXPLAIN SELECT 1 FROM users@home_region_and_address
+WHERE home_region IN ('ap':::STRING, 'ca':::STRING, 'us':::STRING)
+  AND address = '221B Baker Street';
+----
+distribution: local
+vectorized: true
+·
+• render
+│
+└── • scan
+      missing stats
+      table: users@home_region_and_address
+      spans: [/'ap'/'221B Baker Street' - /'ap'/'221B Baker Street'] [/'ca'/'221B Baker Street' - /'ca'/'221B Baker Street'] [/'us'/'221B Baker Street' - /'us'/'221B Baker Street']
+
+# With a hard limit <= the max cardinality of the local scan, we should choose
+# locality-optimized scan.
+query T retry
+EXPLAIN SELECT 1 FROM users@home_region_and_address
+WHERE home_region IN ('ap':::STRING, 'ca':::STRING, 'us':::STRING)
+  AND address = '221B Baker Street' LIMIT 1;
+----
+distribution: local
+vectorized: true
+·
+• render
+│
+└── • union all
+    │ limit: 1
+    │
+    ├── • scan
+    │     missing stats
+    │     table: users@home_region_and_address
+    │     spans: [/'ap'/'221B Baker Street' - /'ap'/'221B Baker Street']
+    │     limit: 1
+    │
+    └── • scan
+          missing stats
+          table: users@home_region_and_address
+          spans: [/'ca'/'221B Baker Street' - /'ca'/'221B Baker Street'] [/'us'/'221B Baker Street' - /'us'/'221B Baker Street']
+          limit: 1
+
+# With a hard limit > the max cardinality of the local scan, we should not
+# choose locality-optimized scan.
+query T retry
+EXPLAIN SELECT 1 FROM users@home_region_and_address
+WHERE home_region IN ('ap':::STRING, 'ca':::STRING, 'us':::STRING)
+  AND address = '221B Baker Street' LIMIT 2;
+----
+distribution: local
+vectorized: true
+·
+• render
+│
+└── • scan
+      missing stats
+      table: users@home_region_and_address
+      spans: [/'ap'/'221B Baker Street' - /'ap'/'221B Baker Street'] [/'ca'/'221B Baker Street' - /'ca'/'221B Baker Street'] [/'us'/'221B Baker Street' - /'us'/'221B Baker Street']
+      limit: 2
+
+statement ok
+ALTER TABLE users ADD UNIQUE WITHOUT INDEX (address)
+
+# With a unique constraint on the non-partitioning index key columns, a
+# non-limited scan can avoid reading remote regions by picking
+# locality-optimized scan.
+query T retry
+EXPLAIN SELECT 1 FROM users@home_region_and_address
+WHERE home_region IN ('ap':::STRING, 'ca':::STRING, 'us':::STRING)
+  AND address = '221B Baker Street';
+----
+distribution: local
+vectorized: true
+·
+• render
+│
+└── • union all
+    │ limit: 1
+    │
+    ├── • scan
+    │     missing stats
+    │     table: users@home_region_and_address
+    │     spans: [/'ap'/'221B Baker Street' - /'ap'/'221B Baker Street']
+    │
+    └── • scan
+          missing stats
+          table: users@home_region_and_address
+          spans: [/'ca'/'221B Baker Street' - /'ca'/'221B Baker Street'] [/'us'/'221B Baker Street' - /'us'/'221B Baker Street']
+
+statement ok
+RESET experimental_enable_unique_without_index_constraints
+
+################################################
+# End regression tests for support issue #1780 #
+################################################

pkg/cmd/roachtest/tests/tpchvec.go

@@ -591,7 +591,7 @@ func registerTPCHVec(r registry.Registry) {
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 			runTPCHVec(ctx, t, c, newTpchVecPerfTest(
 				"sql.distsql.use_streamer.enabled", /* settingName */
-				1.25,                               /* slownessThreshold */
+				1.5,                                /* slownessThreshold */
 			), baseTestRun)
 		},
 	})

pkg/sql/opt/xform/scan_funcs.go

@@ -212,6 +212,26 @@ func (c *CustomFuncs) GenerateLocalityOptimizedScan(
 	localScanPrivate.SetConstraint(c.e.evalCtx, &localConstraint)
 	localScanPrivate.HardLimit = scanPrivate.HardLimit
 	localScan := c.e.f.ConstructScan(localScanPrivate)
+	if scanPrivate.HardLimit != 0 {
+		// If the local scan could never reach the hard limit, we will always have
+		// to read into remote regions, so there is no point in using
+		// locality-optimized scan.
+		if scanPrivate.HardLimit > memo.ScanLimit(localScan.Relational().Cardinality.Max) {
+			return
+		}
+	} else {
+		// When the max cardinality of the original scan is greater than the max
+		// cardinality of the local scan, a remote scan will always be required.
+		// IgnoreUniqueWithoutIndexKeys is true when we're performing a scan
+		// during an insert to verify there are no duplicates violating the
+		// uniqueness constraint. This could cause the check below to return, but
+		// by design we want to use locality-optimized search for these duplicate
+		// checks. So avoid returning if that flag is set.
+		if localScan.Relational().Cardinality.Max <
+			grp.Relational().Cardinality.Max && !tabMeta.IgnoreUniqueWithoutIndexKeys {
+			return
+		}
+	}
 
 	// Create the remote scan.
 	remoteScanPrivate := c.DuplicateScanPrivate(scanPrivate)

pkg/sql/scheduledlogging/BUILD.bazel

@@ -40,6 +40,7 @@ go_test(
         "//pkg/settings/cluster",
         "//pkg/testutils",
         "//pkg/testutils/serverutils",
+        "//pkg/testutils/skip",
         "//pkg/testutils/sqlutils",
         "//pkg/testutils/testcluster",
         "//pkg/util/leaktest",

pkg/sql/scheduledlogging/captured_index_usage_stats.go

@@ -39,23 +39,23 @@ var telemetryCaptureIndexUsageStatsEnabled = settings.RegisterBoolSetting(
 )
 
 var telemetryCaptureIndexUsageStatsInterval = settings.RegisterDurationSetting(
-	settings.SystemOnly,
+	settings.TenantReadOnly,
 	"sql.telemetry.capture_index_usage_stats.interval",
 	"the scheduled interval time between capturing index usage statistics when capturing index usage statistics is enabled",
 	8*time.Hour,
 	settings.NonNegativeDuration,
 )
 
 var telemetryCaptureIndexUsageStatsStatusCheckEnabledInterval = settings.RegisterDurationSetting(
-	settings.SystemOnly,
+	settings.TenantReadOnly,
 	"sql.telemetry.capture_index_usage_stats.check_enabled_interval",
 	"the scheduled interval time between checks to see if index usage statistics has been enabled",
 	10*time.Minute,
 	settings.NonNegativeDuration,
 )
 
 var telemetryCaptureIndexUsageStatsLoggingDelay = settings.RegisterDurationSetting(
-	settings.SystemOnly,
+	settings.TenantReadOnly,
 	"sql.telemetry.capture_index_usage_stats.logging_delay",
 	"the time delay between emitting individual index usage stats logs, this is done to "+
 		"mitigate the log-line limit of 10 logs per second on the telemetry pipeline",
@@ -138,13 +138,23 @@ func Start(
 func (s *CaptureIndexUsageStatsLoggingScheduler) start(ctx context.Context, stopper *stop.Stopper) {
 	_ = stopper.RunAsyncTask(ctx, "capture-index-usage-stats", func(ctx context.Context) {
 		// Start the scheduler immediately.
-		for timer := time.NewTimer(0 * time.Second); ; timer.Reset(s.durationUntilNextInterval()) {
+		timer := time.NewTimer(0 * time.Second)
+		defer timer.Stop()
+
+		for {
 			select {
 			case <-stopper.ShouldQuiesce():
-				timer.Stop()
 				return
 			case <-timer.C:
 				s.currentCaptureStartTime = timeutil.Now()
+				dur := s.durationUntilNextInterval()
+				if dur < time.Second {
+					// Avoid intervals that are too short, to prevent a hot
+					// spot on this task.
+					dur = time.Second
+				}
+				timer.Reset(dur)
+
 				if !telemetryCaptureIndexUsageStatsEnabled.Get(&s.st.SV) {
 					continue
 				}
@@ -180,25 +190,24 @@ func captureIndexUsageStats(
 			continue
 		}
 		stmt := fmt.Sprintf(`
-		SELECT
-		 ti.descriptor_name as table_name,
-		 ti.descriptor_id as table_id,
-		 ti.index_name,
-		 ti.index_id,
-		 ti.index_type,
-		 ti.is_unique,
-		 ti.is_inverted,
-		 total_reads,
-		 last_read,
-		 ti.created_at,
-     t.schema_name
-	  FROM %[1]s.crdb_internal.index_usage_statistics AS us
+  SELECT ti.descriptor_name as table_name,
+         ti.descriptor_id as table_id,
+         ti.index_name,
+         ti.index_id,
+         ti.index_type,
+         ti.is_unique,
+         ti.is_inverted,
+         total_reads,
+         last_read,
+         ti.created_at,
+         t.schema_name
+    FROM %[1]s.crdb_internal.index_usage_statistics us
     JOIN %[1]s.crdb_internal.table_indexes ti
-		ON us.index_id = ti.index_id
-		 AND us.table_id = ti.descriptor_id
-    JOIN %[1]s.crdb_internal.tables t 
-    ON ti.descriptor_id = t.table_id
-		ORDER BY total_reads ASC;`,
+      ON us.index_id = ti.index_id
+     AND us.table_id = ti.descriptor_id
+    JOIN %[1]s.crdb_internal.tables t
+      ON ti.descriptor_id = t.table_id
+ORDER BY total_reads ASC;`,
 			databaseName.String())
 
 		it, err := ie.QueryIteratorEx(
@@ -276,13 +285,12 @@ func captureIndexUsageStats(
 func logIndexUsageStatsWithDelay(
 	ctx context.Context, events []logpb.EventPayload, stopper *stop.Stopper, delay time.Duration,
 ) {
-
 	// Log the first event immediately.
 	timer := time.NewTimer(0 * time.Second)
+	defer timer.Stop()
 	for len(events) > 0 {
 		select {
 		case <-stopper.ShouldQuiesce():
-			timer.Stop()
 			return
 		case <-timer.C:
 			event := events[0]
@@ -292,7 +300,6 @@ func logIndexUsageStatsWithDelay(
 			timer.Reset(delay)
 		}
 	}
-	timer.Stop()
 }
 
 func getAllDatabaseNames(ctx context.Context, ie sqlutil.InternalExecutor) (tree.NameList, error) {