Merge #70586 #70707

70586: sql,server: enable tenant status server to work with persisted SQL stats r=maryliag,dhartunian a=Azhng

Previsouly, tenant status server was not able to work with the new
persisted SQL Stats. This result in SQL Stats RPC not able to return
or reset persisted SQL stats.
This commit addresses this issue to update tenant status server's
implementation to be able to work with persisted SQL Stats.

Resolves #70585 #70529 #68177

Release Justification: Bug fixes and low-risk updates to new functionality

Release note (bug fix): Statement/Transaction Page now is able to
display and reset persisted SQL Stats.

70707: clusterversion,storage: cluster version that uses Pebble's SetWithDelete r=sumeerbhola a=sumeerbhola

This is a backwards incompatible change in Pebble to provide
SingleDelete semantics that are clean and robust to programming
error.

This is being done post v21.2-beta but before GA since this
was deemed a GA blocker and not a release blocker. It allows
for upgrading beta clusters to the GA version. For more on these
discussions see the following threads
cockroachdb/pebble#1255 (comment),
https://cockroachlabs.slack.com/archives/C4A9ALLRL/p1631213490022600
(Cockroach Labs internal link).

Informs cockroachdb/pebble#1255
Informs #69891

Release note: None

Release justification: high-severity bug fix

Co-authored-by: Azhng <archer.xn@gmail.com>
Co-authored-by: sumeerbhola <sumeer@cockroachlabs.com>

docs/generated/http/full.md

@@ -3276,6 +3276,7 @@ Request object for issuing a SQL stats reset request.
 | Field | Type | Label | Description | Support status |
 | ----- | ---- | ----- | ----------- | -------------- |
 | node_id | [string](#cockroach.server.serverpb.ResetSQLStatsRequest-string) |  |  | [reserved](#support-status) |
+| reset_persisted_stats | [bool](#cockroach.server.serverpb.ResetSQLStatsRequest-bool) |  | reset_persisted_stats specifies if the persisted SQL Stats will be reset along with the in-memory SQL stats. | [reserved](#support-status) |
 
 
 

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

@@ -165,4 +165,4 @@ trace.datadog.project	string	CockroachDB	the project under which traces will be
 trace.debug.enable	boolean	false	if set, traces for recent requests can be seen at https://<ui>/debug/requests
 trace.lightstep.token	string		if set, traces go to Lightstep using this token
 trace.zipkin.collector	string		if set, traces go to the given Zipkin instance (example: '127.0.0.1:9411'). Only one tracer can be configured at a time.
-version	version	21.1-1164	set the active cluster version in the format '<major>.<minor>'
+version	version	21.1-1166	set the active cluster version in the format '<major>.<minor>'

docs/generated/settings/settings.html

@@ -170,6 +170,6 @@
 <tr><td><code>trace.debug.enable</code></td><td>boolean</td><td><code>false</code></td><td>if set, traces for recent requests can be seen at https://<ui>/debug/requests</td></tr>
 <tr><td><code>trace.lightstep.token</code></td><td>string</td><td><code></code></td><td>if set, traces go to Lightstep using this token</td></tr>
 <tr><td><code>trace.zipkin.collector</code></td><td>string</td><td><code></code></td><td>if set, traces go to the given Zipkin instance (example: '127.0.0.1:9411'). Only one tracer can be configured at a time.</td></tr>
-<tr><td><code>version</code></td><td>version</td><td><code>21.1-1164</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
+<tr><td><code>version</code></td><td>version</td><td><code>21.1-1166</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
 </tbody>
 </table>

pkg/ccl/serverccl/BUILD.bazel

@@ -2,9 +2,25 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "serverccl",
-    srcs = ["doc.go"],
+    srcs = [
+        "doc.go",
+        "tenant_test_utils.go",
+    ],
     importpath = "github.com/cockroachdb/cockroach/pkg/ccl/serverccl",
     visibility = ["//visibility:public"],
+    deps = [
+        "//pkg/base",
+        "//pkg/roachpb:with-mocks",
+        "//pkg/security",
+        "//pkg/server/serverpb",
+        "//pkg/sql/pgwire",
+        "//pkg/sql/sqlstats/persistedsqlstats",
+        "//pkg/sql/tests",
+        "//pkg/testutils/serverutils",
+        "//pkg/testutils/sqlutils",
+        "//pkg/util/log",
+        "@com_github_stretchr_testify//require",
+    ],
 )
 
 go_test(
@@ -37,6 +53,7 @@ go_test(
         "//pkg/sql/sqlstats",
         "//pkg/sql/tests",
         "//pkg/testutils/serverutils",
+        "//pkg/testutils/skip",
         "//pkg/testutils/sqlutils",
         "//pkg/testutils/testcluster",
         "//pkg/util",

pkg/ccl/serverccl/tenant_status_test.go

@@ -11,6 +11,7 @@ package serverccl
 import (
 	"context"
 	gosql "database/sql"
+	"fmt"
 	"net/url"
 	"sort"
 	"strings"
@@ -25,6 +26,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats"
 	"github.com/cockroachdb/cockroach/pkg/sql/tests"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -189,3 +191,119 @@ func TestTenantCannotSeeNonTenantStats(t *testing.T) {
 		}
 	}
 }
+
+func TestResetSQLStatsRPCForTenant(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	skip.UnderStressRace(t, "expensive tests")
+
+	ctx := context.Background()
+
+	stmts := []string{
+		"SELECT 1",
+		"SELECT 1, 1",
+		"SELECT 1, 1, 1",
+	}
+
+	testHelper := newTestTenantHelper(t, 3 /* tenantClusterSize */)
+	defer testHelper.cleanup(ctx, t)
+
+	testCluster := testHelper.testCluster()
+	controlCluster := testHelper.controlCluster()
+
+	// Disable automatic flush to ensure tests are deterministic.
+	testCluster.tenantConn(0 /* idx */).
+		Exec(t, "SET CLUSTER SETTING sql.stats.flush.enabled = false")
+	controlCluster.tenantConn(0 /* idx */).
+		Exec(t, "SET CLUSTER SETTING sql.stats.flush.enabled = false")
+
+	for _, flushed := range []bool{false, true} {
+		t.Run(fmt.Sprintf("flushed=%t", flushed), func(t *testing.T) {
+			// Clears the SQL Stats at the end of each test via builtin.
+			defer func() {
+				testCluster.tenantConn(0 /* idx */).Exec(t, "SELECT crdb_internal.reset_sql_stats()")
+				controlCluster.tenantConn(0 /* idx */).Exec(t, "SELECT crdb_internal.reset_sql_stats()")
+			}()
+
+			for _, stmt := range stmts {
+				testCluster.tenantConn(0 /* idx */).Exec(t, stmt)
+				controlCluster.tenantConn(0 /* idx */).Exec(t, stmt)
+			}
+
+			if flushed {
+				testCluster.tenantSQLStats(0 /* idx */).Flush(ctx)
+				controlCluster.tenantSQLStats(0 /* idx */).Flush(ctx)
+			}
+
+			status := testCluster.tenantStatusSrv(1 /* idx */)
+
+			statsPreReset, err := status.Statements(ctx, &serverpb.StatementsRequest{
+				Combined: true,
+			})
+			require.NoError(t, err)
+
+			require.NotEqual(t, 0, len(statsPreReset.Statements),
+				"expected to find stats for at least one statement, but found: %d", len(statsPreReset.Statements))
+			ensureExpectedStmtFingerprintExistsInRPCResponse(t, stmts, statsPreReset, "test")
+
+			_, err = status.ResetSQLStats(ctx, &serverpb.ResetSQLStatsRequest{
+				ResetPersistedStats: true,
+			})
+			require.NoError(t, err)
+
+			statsPostReset, err := status.Statements(ctx, &serverpb.StatementsRequest{
+				Combined: true,
+			})
+			require.NoError(t, err)
+
+			if !statsPostReset.LastReset.After(statsPreReset.LastReset) {
+				t.Fatal("expected to find stats last reset value changed, but didn't")
+			}
+
+			for _, txnStatsPostReset := range statsPostReset.Transactions {
+				for _, txnStatsPreReset := range statsPreReset.Transactions {
+					require.NotEqual(t, txnStatsPostReset, txnStatsPreReset,
+						"expected to have reset SQL stats, but still found transaction %+v", txnStatsPostReset)
+				}
+			}
+
+			for _, stmtStatsPostReset := range statsPostReset.Statements {
+				for _, stmtStatsPreReset := range statsPreReset.Statements {
+					require.NotEqual(t, stmtStatsPostReset, stmtStatsPreReset,
+						"expected to have reset SQL stats, but still found statement %+v", stmtStatsPostReset)
+				}
+			}
+
+			// Ensures that sql stats reset is isolated by tenant boundary.
+			statsFromControlCluster, err :=
+				controlCluster.tenantStatusSrv(1 /* idx */).Statements(ctx, &serverpb.StatementsRequest{
+					Combined: true,
+				})
+			require.NoError(t, err)
+
+			ensureExpectedStmtFingerprintExistsInRPCResponse(t, stmts, statsFromControlCluster, "control")
+		})
+	}
+}
+
+func ensureExpectedStmtFingerprintExistsInRPCResponse(
+	t *testing.T, expectedStmts []string, resp *serverpb.StatementsResponse, clusterType string,
+) {
+	t.Helper()
+
+	for _, stmt := range expectedStmts {
+		fingerprint := strings.Replace(stmt, "1", "_", -1)
+		found := false
+		for _, foundStmt := range resp.Statements {
+			if !strings.Contains(foundStmt.Key.KeyData.App, resp.InternalAppNamePrefix) {
+				if fingerprint == foundStmt.Key.KeyData.Query {
+					found = true
+					break
+				}
+			}
+		}
+		require.True(t, found, "expected %s to be found in "+
+			"%s tenant cluster, but it was not found", fingerprint, clusterType)
+	}
+}

pkg/ccl/serverccl/tenant_test_utils.go

@@ -0,0 +1,148 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Licensed as a CockroachDB Enterprise file under the Cockroach Community
+// License (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+//     https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt
+
+package serverccl
+
+import (
+	"context"
+	gosql "database/sql"
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/security"
+	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats"
+	"github.com/cockroachdb/cockroach/pkg/sql/tests"
+	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/stretchr/testify/require"
+)
+
+type testTenant struct {
+	tenant         serverutils.TestTenantInterface
+	tenantConn     *gosql.DB
+	tenantDB       *sqlutils.SQLRunner
+	tenantStatus   serverpb.SQLStatusServer
+	tenantSQLStats *persistedsqlstats.PersistedSQLStats
+}
+
+func newTestTenant(
+	t *testing.T, server serverutils.TestServerInterface, existing bool, tenantID roachpb.TenantID,
+) *testTenant {
+	t.Helper()
+
+	tenantParams := tests.CreateTestTenantParams(tenantID)
+	tenantParams.Existing = existing
+
+	log.TestingClearServerIdentifiers()
+	tenant, tenantConn := serverutils.StartTenant(t, server, tenantParams)
+	sqlDB := sqlutils.MakeSQLRunner(tenantConn)
+	status := tenant.StatusServer().(serverpb.SQLStatusServer)
+	sqlStats := tenant.PGServer().(*pgwire.Server).SQLServer.
+		GetSQLStatsProvider().(*persistedsqlstats.PersistedSQLStats)
+
+	return &testTenant{
+		tenant:         tenant,
+		tenantConn:     tenantConn,
+		tenantDB:       sqlDB,
+		tenantStatus:   status,
+		tenantSQLStats: sqlStats,
+	}
+}
+
+func (h *testTenant) cleanup(t *testing.T) {
+	require.NoError(t, h.tenantConn.Close())
+}
+
+type tenantTestHelper struct {
+	hostCluster serverutils.TestClusterInterface
+
+	// Creating two separate tenant clusters. This allows unit tests to test
+	// the isolation between different tenants are properly enforced.
+	tenantTestCluster    tenantCluster
+	tenantControlCluster tenantCluster
+}
+
+func newTestTenantHelper(t *testing.T, tenantClusterSize int) *tenantTestHelper {
+	t.Helper()
+
+	params, _ := tests.CreateTestServerParams()
+	testCluster := serverutils.StartNewTestCluster(t, 3 /* numNodes */, base.TestClusterArgs{
+		ServerArgs: params,
+	})
+	server := testCluster.Server(0)
+
+	return &tenantTestHelper{
+		hostCluster: testCluster,
+		tenantTestCluster: newTenantCluster(
+			t,
+			server,
+			tenantClusterSize,
+			security.EmbeddedTenantIDs()[0],
+		),
+		tenantControlCluster: newTenantCluster(
+			t,
+			server,
+			tenantClusterSize,
+			security.EmbeddedTenantIDs()[1],
+		),
+	}
+}
+
+func (h *tenantTestHelper) testCluster() tenantCluster {
+	return h.tenantTestCluster
+}
+
+func (h *tenantTestHelper) controlCluster() tenantCluster {
+	return h.tenantControlCluster
+}
+
+func (h *tenantTestHelper) cleanup(ctx context.Context, t *testing.T) {
+	t.Helper()
+	h.hostCluster.Stopper().Stop(ctx)
+	h.tenantTestCluster.cleanup(t)
+	h.tenantControlCluster.cleanup(t)
+}
+
+type tenantCluster []*testTenant
+
+func newTenantCluster(
+	t *testing.T, server serverutils.TestServerInterface, tenantClusterSize int, tenantID uint64,
+) tenantCluster {
+	t.Helper()
+
+	cluster := make([]*testTenant, tenantClusterSize)
+	existing := false
+	for i := 0; i < tenantClusterSize; i++ {
+		cluster[i] = newTestTenant(t, server, existing, roachpb.MakeTenantID(tenantID))
+		existing = true
+	}
+
+	return cluster
+}
+
+func (c tenantCluster) tenantConn(idx int) *sqlutils.SQLRunner {
+	return c[idx].tenantDB
+}
+
+func (c tenantCluster) tenantSQLStats(idx int) *persistedsqlstats.PersistedSQLStats {
+	return c[idx].tenantSQLStats
+}
+
+func (c tenantCluster) tenantStatusSrv(idx int) serverpb.SQLStatusServer {
+	return c[idx].tenantStatus
+}
+
+func (c tenantCluster) cleanup(t *testing.T) {
+	for _, tenant := range c {
+		tenant.cleanup(t)
+	}
+}

pkg/clusterversion/cockroach_versions.go

@@ -281,6 +281,11 @@ const (
 	// MarkerDataKeysRegistry switches to using an atomic marker file
 	// for denoting which data keys registry is active.
 	MarkerDataKeysRegistry
+	// PebbleSetWithDelete switches to a backwards incompatible Pebble version
+	// that provides SingleDelete semantics that are cleaner and robust to
+	// programming error. See https://github.com/cockroachdb/pebble/issues/1255
+	// and #69891.
+	PebbleSetWithDelete
 
 	// *************************************************
 	// Step (1): Add new versions here.
@@ -471,6 +476,10 @@ var versionsSingleton = keyedVersions{
 		Key:     MarkerDataKeysRegistry,
 		Version: roachpb.Version{Major: 21, Minor: 1, Internal: 1164},
 	},
+	{
+		Key:     PebbleSetWithDelete,
+		Version: roachpb.Version{Major: 21, Minor: 1, Internal: 1166},
+	},
 	// *************************************************
 	// Step (2): Add new versions here.
 	// Do not add new versions to a patch release.

pkg/rpc/auth_tenant.go

@@ -39,7 +39,7 @@ func tenantFromCommonName(commonName string) (roachpb.TenantID, error) {
 }
 
 // authorize enforces a security boundary around endpoints that tenants
-// request from the host KV node.
+// request from the host KV node or other tenant SQL pod.
 func (a tenantAuthorizer) authorize(
 	tenID roachpb.TenantID, fullMethod string, req interface{},
 ) error {
@@ -68,6 +68,9 @@ func (a tenantAuthorizer) authorize(
 	case "/cockroach.server.serverpb.Status/Statements":
 		return a.authTenant(tenID)
 
+	case "/cockroach.server.serverpb.Status/ResetSQLStats":
+		return a.authTenant(tenID)
+
 	case "/cockroach.server.serverpb.Status/ListSessions":
 		return a.authTenant(tenID)