Merge #61041 #61158

61041: multiregionccl: gate multi-region database configuration behind a license r=ajstorm a=otan Resolves #59668 Release justification: low risk changes to new functionality Release note (enterprise change): Multi-region database creations are permitted as long as the cluster has a CockroachDB subscription. 61158: tracing: do not set tags when setting stats r=yuzefovich a=yuzefovich **colflow: do not set redundant tag on the tracing span** This is unnecessary given that we're setting the componentID below that includes the flowID. Release justification: low-risk update to new functionality. Release note: None **tracing: do not set tags when setting stats** We no longer need to set tags on the tracing span in order to propagate stats since we now propagate that info as a whole object (at the moment, twice - as a Structured payload and as DeprecatedStats protobuf; the latter will be removed after 21.1 branch is cut). Addresses: #59379. Release justification: low-risk update to new functionality. Release note: None Co-authored-by: Oliver Tan <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]>
cockroachdb · Feb 26, 2021 · e4987e4 · e4987e4
3 parents fe1192c + c0fda09 + d37ee52
commit e4987e4
Show file tree

Hide file tree

Showing 10 changed files with 165 additions and 48 deletions.
diff --git a/pkg/ccl/multiregionccl/BUILD.bazel b/pkg/ccl/multiregionccl/BUILD.bazel
@@ -6,6 +6,7 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl",
     visibility = ["//visibility:public"],
     deps = [
+        "//pkg/ccl/utilccl",
         "//pkg/clusterversion",
         "//pkg/sql",
         "//pkg/sql/catalog/catalogkv",
@@ -20,6 +21,7 @@ go_test(
     name = "multiregionccl_test",
     srcs = [
         "main_test.go",
+        "multiregion_test.go",
         "region_test.go",
         "regional_by_row_test.go",
         "show_test.go",
@@ -41,6 +43,7 @@ go_test(
         "//pkg/sql/tests",
         "//pkg/testutils",
         "//pkg/testutils/serverutils",
+        "//pkg/testutils/skip",
         "//pkg/testutils/testcluster",
         "//pkg/util",
         "//pkg/util/leaktest",

diff --git a/pkg/ccl/multiregionccl/multiregion.go b/pkg/ccl/multiregionccl/multiregion.go
@@ -12,6 +12,7 @@ import (
 	"context"
 	"sort"
 
+	"github.com/cockroachdb/cockroach/pkg/ccl/utilccl"
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
@@ -38,6 +39,15 @@ func createRegionConfig(
 		return descpb.DatabaseDescriptor_RegionConfig{}, err
 	}
 
+	if err := utilccl.CheckEnterpriseEnabled(
+		execCfg.Settings,
+		execCfg.ClusterID(),
+		execCfg.Organization(),
+		"multi-region features",
+	); err != nil {
+		return descpb.DatabaseDescriptor_RegionConfig{}, err
+	}
+
 	var regionConfig descpb.DatabaseDescriptor_RegionConfig
 	var err error
 	regionConfig.SurvivalGoal, err = sql.TranslateSurvivalGoal(survivalGoal)

diff --git a/pkg/ccl/multiregionccl/multiregion_test.go b/pkg/ccl/multiregionccl/multiregion_test.go
@@ -0,0 +1,101 @@
+// 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 multiregionccl_test
+
+import (
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/ccl/utilccl"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/stretchr/testify/require"
+)
+
+const multiRegionNoEnterpriseContains = "use of multi-region features requires an enterprise license"
+
+func TestMultiRegionNoLicense(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+	defer utilccl.TestingDisableEnterprise()()
+
+	_, sqlDB, cleanup := createTestMultiRegionCluster(t, 3, base.TestingKnobs{})
+	defer cleanup()
+
+	_, err := sqlDB.Exec(`CREATE DATABASE test`)
+	require.NoError(t, err)
+
+	for _, errorStmt := range []string{
+		`CREATE DATABASE db WITH PRIMARY REGION "us-east1" REGIONS "us-east2"`,
+		`ALTER DATABASE test PRIMARY REGION "us-east2"`,
+	} {
+		t.Run(errorStmt, func(t *testing.T) {
+			_, err := sqlDB.Exec(errorStmt)
+			require.Error(t, err)
+			require.Contains(t, err.Error(), multiRegionNoEnterpriseContains)
+		})
+	}
+}
+
+func TestMultiRegionAfterEnterpriseDisabled(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+	defer utilccl.TestingEnableEnterprise()()
+
+	skip.UnderRace(t, "#61163")
+
+	_, sqlDB, cleanup := createTestMultiRegionCluster(t, 3, base.TestingKnobs{})
+	defer cleanup()
+
+	_, err := sqlDB.Exec(`
+CREATE DATABASE test PRIMARY REGION "us-east1" REGIONS "us-east2";
+USE test;
+CREATE TABLE t1 () LOCALITY GLOBAL;
+CREATE TABLE t2 () LOCALITY REGIONAL BY TABLE;
+CREATE TABLE t3 () LOCALITY REGIONAL BY TABLE IN "us-east2";
+CREATE TABLE t4 () LOCALITY REGIONAL BY ROW
+	`)
+	require.NoError(t, err)
+
+	defer utilccl.TestingDisableEnterprise()()
+
+	// Test certain commands are no longer usable.
+	t.Run("no new multi-region items", func(t *testing.T) {
+		for _, errorStmt := range []string{
+			`CREATE DATABASE db WITH PRIMARY REGION "us-east1" REGIONS "us-east2"`,
+		} {
+			t.Run(errorStmt, func(t *testing.T) {
+				_, err := sqlDB.Exec(errorStmt)
+				require.Error(t, err)
+				require.Contains(t, err.Error(), multiRegionNoEnterpriseContains)
+			})
+		}
+	})
+
+	// Test we can still drop multi-region functionality.
+	t.Run("drop multi-region", func(t *testing.T) {
+		for _, tc := range []struct {
+			stmt string
+		}{
+			{stmt: `ALTER DATABASE test PRIMARY REGION "us-east2"`},
+			{stmt: `ALTER TABLE t2 SET LOCALITY REGIONAL BY TABLE`},
+			{stmt: `ALTER TABLE t3 SET LOCALITY REGIONAL BY TABLE`},
+			{stmt: `ALTER TABLE t4 SET LOCALITY REGIONAL BY TABLE`},
+			{stmt: `ALTER TABLE t4 DROP COLUMN crdb_region`},
+			{stmt: `ALTER DATABASE test DROP REGION "us-east1"`},
+			{stmt: `ALTER DATABASE test DROP REGION "us-east2"`},
+		} {
+			t.Run(tc.stmt, func(t *testing.T) {
+				_, err := sqlDB.Exec(tc.stmt)
+				require.NoError(t, err)
+			})
+		}
+	})
+}
diff --git a/pkg/ccl/multiregionccl/regional_by_row_test.go b/pkg/ccl/multiregionccl/regional_by_row_test.go
@@ -33,9 +33,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// REGIONAL BY ROW tests are defined in multiregionccl as REGIONAL BY ROW
-// requires CCL to operate.
-
 // createTestMultiRegionCluster creates a test cluster with numServers number of
 // nodes with the provided testing knobs applied to each of the nodes. Every
 // node is placed in its own locality, named "us-east1", "us-east2", and so on.

diff --git a/pkg/sql/colflow/vectorized_flow.go b/pkg/sql/colflow/vectorized_flow.go
@@ -960,7 +960,6 @@ func (s *vectorizedFlowCreator) setupOutput(
 							// At the last outbox, we can accurately retrieve stats for the
 							// whole flow from parent monitors. These stats are added to a
 							// flow-level span.
-							span.SetTag(execinfrapb.FlowIDTagKey, flowCtx.ID)
 							span.SetSpanStats(&execinfrapb.ComponentStats{
 								Component: execinfrapb.FlowComponentID(base.SQLInstanceID(outputStream.OriginNodeID), flowCtx.ID),
 								FlowStats: execinfrapb.FlowStats{

diff --git a/pkg/sql/opt/exec/execbuilder/testdata/select b/pkg/sql/opt/exec/execbuilder/testdata/select
@@ -15,31 +15,29 @@ SET vectorize=off; SET tracing = on; BEGIN; SELECT 1; COMMIT; SELECT 2; SET trac
 # how many commands we ran in the session.
 query ITT
 SELECT
-  span, regexp_replace(regexp_replace(message, 'pos:[0-9]*', 'pos:?'), 'flowid: [0-9A-Fa-f-]*', 'flowid: ?'), operation
+  span, regexp_replace(message, 'pos:[0-9]*', 'pos:?'), operation
 FROM [SHOW TRACE FOR SESSION]
 WHERE message LIKE '%SPAN START%' OR message LIKE '%pos%executing%';
 ----
-0                    === SPAN START: session recording ===                session recording
-1                    === SPAN START: exec stmt ===                        exec stmt
-1                    [NoTxn pos:?] executing ExecStmt: BEGIN TRANSACTION  exec stmt
-2                    === SPAN START: sql txn ===                          sql txn
-3                    === SPAN START: exec stmt ===                        exec stmt
-3                    [Open pos:?] executing ExecStmt: SELECT 1            exec stmt
-4                    === SPAN START: consuming rows ===                   consuming rows
-5                    === SPAN START: flow ===
-cockroach.flowid: ?  flow
-6                    === SPAN START: exec stmt ===                        exec stmt
-6                    [Open pos:?] executing ExecStmt: COMMIT TRANSACTION  exec stmt
-7                    === SPAN START: exec stmt ===                        exec stmt
-7                    [NoTxn pos:?] executing ExecStmt: SELECT 2           exec stmt
-8                    === SPAN START: sql txn ===                          sql txn
-9                    === SPAN START: exec stmt ===                        exec stmt
-9                    [Open pos:?] executing ExecStmt: SELECT 2            exec stmt
-10                   === SPAN START: consuming rows ===                   consuming rows
-11                   === SPAN START: flow ===
-cockroach.flowid: ?  flow
-12                   === SPAN START: exec stmt ===                        exec stmt
-12                   [NoTxn pos:?] executing ExecStmt: SET TRACING = off  exec stmt
+0   === SPAN START: session recording ===                session recording
+1   === SPAN START: exec stmt ===                        exec stmt
+1   [NoTxn pos:?] executing ExecStmt: BEGIN TRANSACTION  exec stmt
+2   === SPAN START: sql txn ===                          sql txn
+3   === SPAN START: exec stmt ===                        exec stmt
+3   [Open pos:?] executing ExecStmt: SELECT 1            exec stmt
+4   === SPAN START: consuming rows ===                   consuming rows
+5   === SPAN START: flow ===                             flow
+6   === SPAN START: exec stmt ===                        exec stmt
+6   [Open pos:?] executing ExecStmt: COMMIT TRANSACTION  exec stmt
+7   === SPAN START: exec stmt ===                        exec stmt
+7   [NoTxn pos:?] executing ExecStmt: SELECT 2           exec stmt
+8   === SPAN START: sql txn ===                          sql txn
+9   === SPAN START: exec stmt ===                        exec stmt
+9   [Open pos:?] executing ExecStmt: SELECT 2            exec stmt
+10  === SPAN START: consuming rows ===                   consuming rows
+11  === SPAN START: flow ===                             flow
+12  === SPAN START: exec stmt ===                        exec stmt
+12  [NoTxn pos:?] executing ExecStmt: SET TRACING = off  exec stmt
 
 # ------------------------------------------------------------------------------
 # Numeric References Tests.

diff --git a/pkg/sql/rowflow/BUILD.bazel b/pkg/sql/rowflow/BUILD.bazel
@@ -58,5 +58,7 @@ go_test(
         "//pkg/util/randutil",
         "//pkg/util/tracing",
         "@com_github_cockroachdb_errors//:errors",
+        "@com_github_gogo_protobuf//types",
+        "@com_github_stretchr_testify//require",
     ],
 )
diff --git a/pkg/sql/rowflow/routers_test.go b/pkg/sql/rowflow/routers_test.go
@@ -15,7 +15,6 @@ import (
 	"context"
 	"fmt"
 	"math"
-	"strings"
 	"sync"
 	"sync/atomic"
 	"testing"
@@ -39,6 +38,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
 	"github.com/cockroachdb/errors"
+	pbtypes "github.com/gogo/protobuf/types"
+	"github.com/stretchr/testify/require"
 )
 
 // setupRouter creates and starts a router. Returns the router and a WaitGroup
@@ -870,25 +871,34 @@ func TestRouterDiskSpill(t *testing.T) {
 					}
 					traceMetaSeen = true
 					span := meta.TraceData[0]
-					getTagValue := func(key string) string {
-						strValue, ok := span.Tags[key]
-						if !ok {
-							t.Errorf("missing tag: %s", key)
+					var stats execinfrapb.ComponentStats
+					var err error
+					var unmarshalled bool
+					span.Structured(func(any *pbtypes.Any) {
+						if !pbtypes.Is(any, &stats) {
+							return
 						}
-						return strValue
-					}
-					t.Logf("tags: %v\n", span.Tags)
-					rowsRouted := getTagValue("cockroach.stat.input.tuples")
-					memMax := getTagValue("cockroach.stat.max.memory.allocated")
-					diskMax := getTagValue("cockroach.stat.max.scratch.disk.allocated")
-					if rowsRouted != fmt.Sprintf("%d", numRows) {
-						t.Errorf("expected %d rows routed, got %s", numRows, rowsRouted)
+						if err = pbtypes.UnmarshalAny(any, &stats); err != nil {
+							return
+						}
+						unmarshalled = true
+					})
+					require.NoError(t, err)
+					require.True(t, unmarshalled)
+					require.True(t, stats.Inputs[0].NumTuples.HasValue())
+					require.True(t, stats.Exec.MaxAllocatedMem.HasValue())
+					require.True(t, stats.Exec.MaxAllocatedDisk.HasValue())
+					rowsRouted := stats.Inputs[0].NumTuples.Value()
+					memMax := stats.Exec.MaxAllocatedMem.Value()
+					diskMax := stats.Exec.MaxAllocatedDisk.Value()
+					if rowsRouted != numRows {
+						t.Errorf("expected %d rows routed, got %d", numRows, rowsRouted)
 					}
-					if strings.HasPrefix(memMax, `"0"`) {
-						t.Errorf("expected memMax > 0, got %s", memMax)
+					if memMax == 0 {
+						t.Errorf("expected memMax > 0, got %d", memMax)
 					}
-					if strings.HasPrefix(diskMax, `"0"`) {
-						t.Errorf("expected diskMax > 0, got %s", diskMax)
+					if diskMax == 0 {
+						t.Errorf("expected diskMax > 0, got %d", diskMax)
 					}
 				}
 				continue

diff --git a/pkg/sql/trace_test.go b/pkg/sql/trace_test.go
@@ -107,10 +107,10 @@ func TestTrace(t *testing.T) {
 
 				// Check that stat collection from the above SELECT statement is output
 				// to trace. We don't insert any rows in this test, thus the expected
-				// stat value is 0.
+				// num tuples value plus one is 1.
 				rows, err := sqlDB.Query(
 					"SELECT count(message) FROM crdb_internal.session_trace " +
-						"WHERE message LIKE '%cockroach.stat.kv.tuples.read: 0%'",
+						"WHERE message LIKE '%component%num_tuples%value_plus_one:1%'",
 				)
 				if err != nil {
 					t.Fatal(err)

diff --git a/pkg/util/tracing/span.go b/pkg/util/tracing/span.go
@@ -314,9 +314,6 @@ func (s *spanInner) SetSpanStats(stats SpanStats) {
 	s.RecordStructured(stats)
 	s.crdb.mu.Lock()
 	s.crdb.mu.stats = stats
-	for name, value := range stats.StatsTags() {
-		s.setTagInner(name, value, true /* locked */)
-	}
 	s.crdb.mu.Unlock()
 }