Merge #98527 #98879 #98988 #99042 #99054 #99119

98527: sql: deprecate gossip-based physical planning r=dt a=dt

Previously system tenants and secondary tenants used differnt physical planning implementations, with the system tenant and only the system tenant using nodeIDs while other tenants used the instance table. This unifies those implementations such that all tenants use NodeIDs if running in mixed mode and use the instance table if not.

Release note: none.
Epic: CRDB-16910

98879: sql: support array-flatten subqueries within UDFs r=mgartner a=mgartner

Array-flatten subqueries (e.g., `ARRAY(SELECT a FROM t)`) are now
supported within UDFs. They are now converted to a normal subquery with
a ScalarGroupBy if they exist within a UDF, even if they are
uncorrelated. This allows them to be executed without any changes to the
execbuilder or the evaluation logic of `tree.Routine`.

Fixes #98738

Release note: None


98988: ui: add badges for filter elements r=maryliag a=maryliag

Adds badges for each of the selected filters on SQL Activity and
Insights pages.

Part Of #98891

https://www.loom.com/share/e7417209fc704d71b2733f58609fb4de

<img width="1160" alt="Screenshot 2023-03-19 at 1 30 49 PM" src="https://user-images.githubusercontent.com/1017486/226195412-03d3ef58-5d6d-4c24-84f1-6a55b952f5c0.png">

Release note (ui change): Adds badges for each selected
filter on SQL Activity and Insights pages.

99042: sql: added indexes to system.statement_statistics, system.transaction_statistics r=ericharmeling a=ericharmeling

Fixes #98624.

This commit adds indexes on new computed columns to the system.statement_statistics and system.transaction_statistics tables.

Epic: none

Release note: None

99054: pkg/ccl: Unskip TestTenantStatusAPI/tenant_ranges/pagination r=dhartunian a=abarganier

Fixes: #92979

Previously, in #97386, we skipped test_tenant_ranges_pagination because it was marked as flaky.

The test makes a request for a single range and expects an offset of `1` back. It then uses this offset to request a second range, and expects an offset of `2`. This means that the test requires at least 3 ranges to exist on the tenant.

The test was flaking on the assertion that the offset returned by the second request came back as `2`. Instead, it was flaking when the offset came back as `0`, which signifies that there are no more ranges to process.

We learned that the tenant create process has an asycnhronous splitting of ranges that occurs, which is what would lead to this sporadic scenario where not enough ranges existed (yet) for the test to succeed.

This patch updates the test with a `testutils.SucceedsSoon` clause that checks first that `crdb_internal.ranges` contains at least 3 ranges, prior to making the second request. This should provide sufficient time for the range split queue to be processed and eliminate the vast majority of these test flakes.

Release note: none

99119: kvserver: mark in-flight storage checkpoints r=tbg a=pavelkalinnikov

This commit makes it so that the consistency checker checkpoints are first created in a "_pending" folder, and only after completion are atomically renamed to the intended directory name. This is to help distinguish valid checkpoints from the ones that weren't finalized (for example, when the node crashed in the meantime).

Part of #81819
Epic: none
Release note (ops change): checkpoint directories (that can be created in the rare event of range inconsistency) are now clearly indicated as pending until they are fully populated. This is to help operators to distinguish valid checkpoints from corrupted ones.

Co-authored-by: David Taylor <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: maryliag <[email protected]>
Co-authored-by: Eric Harmeling <[email protected]>
Co-authored-by: Alex Barganier <[email protected]>
Co-authored-by: Pavel Kalinnikov <[email protected]>

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

@@ -289,4 +289,4 @@ trace.opentelemetry.collector	string		address of an OpenTelemetry trace collecto
 trace.snapshot.rate	duration	0s	if non-zero, interval at which background trace snapshots are captured
 trace.span_registry.enabled	boolean	true	if set, ongoing traces can be seen at https://<ui>/#/debug/tracez
 trace.zipkin.collector	string		the address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used.
-version	version	1000022.2-90	set the active cluster version in the format '<major>.<minor>'
+version	version	1000022.2-92	set the active cluster version in the format '<major>.<minor>'

docs/generated/settings/settings.html

@@ -241,6 +241,6 @@
 <tr><td><div id="setting-trace-snapshot-rate" class="anchored"><code>trace.snapshot.rate</code></div></td><td>duration</td><td><code>0s</code></td><td>if non-zero, interval at which background trace snapshots are captured</td></tr>
 <tr><td><div id="setting-trace-span-registry-enabled" class="anchored"><code>trace.span_registry.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>if set, ongoing traces can be seen at https://&lt;ui&gt;/#/debug/tracez</td></tr>
 <tr><td><div id="setting-trace-zipkin-collector" class="anchored"><code>trace.zipkin.collector</code></div></td><td>string</td><td><code></code></td><td>the address of a Zipkin instance to receive traces, as &lt;host&gt;:&lt;port&gt;. If no port is specified, 9411 will be used.</td></tr>
-<tr><td><div id="setting-version" class="anchored"><code>version</code></div></td><td>version</td><td><code>1000022.2-90</code></td><td>set the active cluster version in the format &#39;&lt;major&gt;.&lt;minor&gt;&#39;</td></tr>
+<tr><td><div id="setting-version" class="anchored"><code>version</code></div></td><td>version</td><td><code>1000022.2-92</code></td><td>set the active cluster version in the format &#39;&lt;major&gt;.&lt;minor&gt;&#39;</td></tr>
 </tbody>
 </table>

pkg/ccl/serverccl/statusccl/tenant_status_test.go

@@ -1290,8 +1290,6 @@ func testTenantRangesRPC(_ context.Context, t *testing.T, helper serverccl.Tenan
 	})
 
 	t.Run("test tenant ranges pagination", func(t *testing.T) {
-		skip.WithIssue(t, 92979,
-			"flaky test, difficult to reproduce locally. Skip until resolved.")
 		ctx := context.Background()
 		resp1, err := tenantA.TenantRanges(ctx, &serverpb.TenantRangesRequest{
 			Limit: 1,
@@ -1302,18 +1300,34 @@ func testTenantRangesRPC(_ context.Context, t *testing.T, helper serverccl.Tenan
 			require.Len(t, ranges.Ranges, 1)
 		}
 
-		resp2, err := tenantA.TenantRanges(ctx, &serverpb.TenantRangesRequest{
-			Limit:  1,
-			Offset: resp1.Next,
+		sql := helper.TestCluster().TenantConn(0)
+		// Wait for the split queue to process some before requesting the 2nd range.
+		// We expect an offset for the 3rd range, so wait until at least 3 ranges exist.
+		testutils.SucceedsSoon(t, func() error {
+			res := sql.QueryStr(t, "SELECT count(*) FROM crdb_internal.ranges")
+			require.Equal(t, len(res), 1)
+			require.Equal(t, len(res[0]), 1)
+			rangeCount, err := strconv.Atoi(res[0][0])
+			require.NoError(t, err)
+			if rangeCount < 3 {
+				return errors.Newf("expected >= 3 ranges, got %d", rangeCount)
+			}
+
+			resp2, err := tenantA.TenantRanges(ctx, &serverpb.TenantRangesRequest{
+				Limit:  1,
+				Offset: resp1.Next,
+			})
+			require.NoError(t, err)
+			require.Equal(t, 2, int(resp2.Next))
+			for locality, ranges := range resp2.RangesByLocality {
+				require.Len(t, ranges.Ranges, 1)
+				// Verify pagination functions based on ascending RangeID order.
+				require.True(t,
+					resp1.RangesByLocality[locality].Ranges[0].RangeID < ranges.Ranges[0].RangeID)
+			}
+			return nil
 		})
-		require.NoError(t, err)
-		require.Equal(t, 2, int(resp2.Next))
-		for locality, ranges := range resp2.RangesByLocality {
-			require.Len(t, ranges.Ranges, 1)
-			// Verify pagination functions based on ascending RangeID order.
-			require.True(t,
-				resp1.RangesByLocality[locality].Ranges[0].RangeID < ranges.Ranges[0].RangeID)
-		}
+
 	})
 }
 

pkg/cli/testdata/declarative-rules/deprules

@@ -1,6 +1,6 @@
 dep
 ----
-debug declarative-print-rules 1000022.2-90 dep
+debug declarative-print-rules 1000022.2-92 dep
 deprules
 ----
 - name: 'CheckConstraint transitions to ABSENT uphold 2-version invariant: PUBLIC->VALIDATED'

pkg/cli/testdata/declarative-rules/oprules

@@ -1,6 +1,6 @@
 op
 ----
-debug declarative-print-rules 1000022.2-90 op
+debug declarative-print-rules 1000022.2-92 op
 rules
 ----
 []

pkg/clusterversion/cockroach_versions.go

@@ -498,6 +498,12 @@ const (
 	// config runner persistent job has been created.
 	V23_1_CreateAutoConfigRunnerJob
 
+	// V23_1AddSQLStatsComputedIndexes is the version at which Cockroach adds new
+	// computed columns and indexes to the statement_statistics and
+	// transaction_statistics system tables. These columns optimize persisted SQL
+	// statistics queries for observability.
+	V23_1AddSQLStatsComputedIndexes
+
 	// *************************************************
 	// Step (1): Add new versions here.
 	// Do not add new versions to a patch release.
@@ -863,6 +869,10 @@ var rawVersionsSingleton = keyedVersions{
 		Key:     V23_1_CreateAutoConfigRunnerJob,
 		Version: roachpb.Version{Major: 22, Minor: 2, Internal: 90},
 	},
+	{
+		Key:     V23_1AddSQLStatsComputedIndexes,
+		Version: roachpb.Version{Major: 22, Minor: 2, Internal: 92},
+	},
 
 	// *************************************************
 	// Step (2): Add new versions here.

pkg/kv/kvserver/consistency_queue_test.go

@@ -17,6 +17,7 @@ import (
 	"math/rand"
 	"path/filepath"
 	"strconv"
+	"strings"
 	"testing"
 	"time"
 
@@ -373,6 +374,10 @@ func TestCheckConsistencyInconsistent(t *testing.T) {
 	for i := 0; i < numStores; i++ {
 		cps := onDiskCheckpointPaths(i)
 		require.Len(t, cps, 1)
+		t.Logf("found a checkpoint at %s", cps[0])
+		// The checkpoint must have been finalized.
+		require.False(t, strings.HasSuffix(cps[0], "_pending"))
+
 		metric := tc.GetFirstStoreFromServer(t, i).Metrics().RdbCheckpoints
 		testutils.SucceedsSoon(t, func() error {
 			if got, want := metric.Value(), int64(1); got != want {
@@ -387,7 +392,8 @@ func TestCheckConsistencyInconsistent(t *testing.T) {
 		require.NoError(t, err)
 		// Copy the min-version file so we can open the checkpoint as a store.
 		require.NoError(t, vfs.Copy(fs, storage.MinVersionFilename, fs.PathJoin(cps[0], storage.MinVersionFilename)))
-		cpEng := storage.InMemFromFS(context.Background(), fs, cps[0], cluster.MakeClusterSettings(), storage.CacheSize(1<<20))
+		cpEng := storage.InMemFromFS(context.Background(), fs, cps[0], cluster.MakeClusterSettings(),
+			storage.ForTesting, storage.MustExist, storage.ReadOnly, storage.CacheSize(1<<20))
 		defer cpEng.Close()
 
 		// Find the problematic range in the storage.

pkg/kv/kvserver/replica_consistency.go

@@ -777,6 +777,10 @@ To inspect the checkpoints, one can use the cockroach debug range-data tool, and
 command line tools like diff. For example:
 
 $ cockroach debug range-data --replicated data/auxiliary/checkpoints/rN_at_M N
+
+Note that a directory that ends with "_pending" might not represent a valid
+checkpoint. Such directories can exist if the node fails during checkpoint
+creation. These directories should be deleted, or inspected with caution.
 `
 		attentionArgs := []any{r, desc.Replicas(), redact.Safe(auxDir), redact.Safe(path)}
 		preventStartupMsg := fmt.Sprintf(attentionFmt, attentionArgs...)

pkg/kv/kvserver/store.go

@@ -3084,8 +3084,15 @@ func (s *Store) checkpointSpans(desc *roachpb.RangeDescriptor) []roachpb.Span {
 func (s *Store) checkpoint(tag string, spans []roachpb.Span) (string, error) {
 	checkpointBase := s.checkpointsDir()
 	_ = s.TODOEngine().MkdirAll(checkpointBase)
+	// Create the checkpoint in a "pending" directory first. If we fail midway, it
+	// should be clear that the directory contains an incomplete checkpoint.
+	pendingDir := filepath.Join(checkpointBase, tag+"_pending")
+	if err := s.TODOEngine().CreateCheckpoint(pendingDir, spans); err != nil {
+		return "", err
+	}
+	// Atomically rename the directory when it represents a complete checkpoint.
 	checkpointDir := filepath.Join(checkpointBase, tag)
-	if err := s.TODOEngine().CreateCheckpoint(checkpointDir, spans); err != nil {
+	if err := s.TODOEngine().Rename(pendingDir, checkpointDir); err != nil {
 		return "", err
 	}
 	return checkpointDir, nil