sql: add id column to system.users and system.role_id_seq

In preparation for keying roles by ids. See RFC: cockroachdb#77453 Release note: None Co-authored-by: Fenil-P <[email protected]>
RichardJCai · Jul 26, 2022 · 3ba7d06 · 3ba7d06
1 parent 9383292
commit 3ba7d06
Show file tree

Hide file tree

Showing 79 changed files with 1,487 additions and 337 deletions.
diff --git a/docs/generated/settings/settings-for-tenants.txt b/docs/generated/settings/settings-for-tenants.txt
@@ -283,4 +283,4 @@ trace.jaeger.agent	string		the address of a Jaeger agent to receive traces using
 trace.opentelemetry.collector	string		address of an OpenTelemetry trace collector to receive traces using the otel gRPC protocol, as <host>:<port>. If no port is specified, 4317 will be used.
 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	22.1-30	set the active cluster version in the format '<major>.<minor>'
+version	version	22.1-36	set the active cluster version in the format '<major>.<minor>'
diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html
@@ -214,6 +214,6 @@
 <tr><td><code>trace.opentelemetry.collector</code></td><td>string</td><td><code></code></td><td>address of an OpenTelemetry trace collector to receive traces using the otel gRPC protocol, as <host>:<port>. If no port is specified, 4317 will be used.</td></tr>
 <tr><td><code>trace.span_registry.enabled</code></td><td>boolean</td><td><code>true</code></td><td>if set, ongoing traces can be seen at https://<ui>/#/debug/tracez</td></tr>
 <tr><td><code>trace.zipkin.collector</code></td><td>string</td><td><code></code></td><td>the address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used.</td></tr>
-<tr><td><code>version</code></td><td>version</td><td><code>22.1-30</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>22.1-36</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
 </tbody>
 </table>
diff --git a/pkg/bench/rttanalysis/testdata/benchmark_expectations b/pkg/bench/rttanalysis/testdata/benchmark_expectations
@@ -35,9 +35,9 @@ exp,benchmark
 16,DropDatabase/drop_database_1_table
 17,DropDatabase/drop_database_2_tables
 18,DropDatabase/drop_database_3_tables
-18,DropRole/drop_1_role
-25,DropRole/drop_2_roles
-32,DropRole/drop_3_roles
+19,DropRole/drop_1_role
+27,DropRole/drop_2_roles
+35,DropRole/drop_3_roles
 17,DropSequence/drop_1_sequence
 19,DropSequence/drop_2_sequences
 21,DropSequence/drop_3_sequences

diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go
@@ -73,6 +73,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/bootstrap"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils"
@@ -1069,14 +1070,17 @@ func TestBackupRestoreSystemTables(t *testing.T) {
 
 	// At the time this test was written, these were the only system tables that
 	// were reasonable for a user to backup and restore into another cluster.
-	tables := []string{"locations", "role_members", "users", "zones"}
+	tables := []string{"locations", "role_members", "users", "zones", "role_id_seq"}
 	tableSpec := "system." + strings.Join(tables, ", system.")
 
 	// Take a consistent fingerprint of the original tables.
 	var backupAsOf string
 	expectedFingerprints := map[string][][]string{}
 	err := crdb.ExecuteTx(ctx, conn, nil /* txopts */, func(tx *gosql.Tx) error {
 		for _, table := range tables {
+			if table == "role_id_seq" {
+				continue
+			}
 			rows, err := conn.Query("SHOW EXPERIMENTAL_FINGERPRINTS FROM TABLE system." + table)
 			if err != nil {
 				return err
@@ -1102,6 +1106,9 @@ func TestBackupRestoreSystemTables(t *testing.T) {
 
 	// Verify the fingerprints match.
 	for _, table := range tables {
+		if table == "role_id_seq" {
+			continue
+		}
 		a := sqlDB.QueryStr(t, "SHOW EXPERIMENTAL_FINGERPRINTS FROM TABLE system_new."+table)
 		if e := expectedFingerprints[table]; !reflect.DeepEqual(e, a) {
 			t.Fatalf("fingerprints between system.%[1]s and system_new.%[1]s did not match:%s\n",
@@ -9508,6 +9515,7 @@ func TestExcludeDataFromBackupDoesNotHoldupGC(t *testing.T) {
 func TestBackupRestoreSystemUsers(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
+	skip.WithIssue(t, 78963)
 
 	sqlDB, tempDir, cleanupFn := createEmptyCluster(t, singleNode)
 	_, sqlDBRestore, cleanupEmptyCluster := backupRestoreTestSetupEmpty(t, singleNode, tempDir, InitManualReplication, base.TestClusterArgs{})
@@ -9535,7 +9543,7 @@ func TestBackupRestoreSystemUsers(t *testing.T) {
 
 		// Role 'app_role' and user 'app' will be added, and 'app' is granted with 'app_role'
 		// User test will remain untouched with no role granted
-		sqlDBRestore.CheckQueryResults(t, "SELECT * FROM system.users", [][]string{
+		sqlDBRestore.CheckQueryResults(t, "SELECT username, \"hashedPassword\", \"isRole\" FROM system.users", [][]string{
 			{"admin", "", "true"},
 			{"app", "NULL", "false"},
 			{"app_role", "NULL", "true"},
@@ -9569,15 +9577,15 @@ func TestBackupRestoreSystemUsers(t *testing.T) {
 	t.Run("restore-from-backup-with-no-system-role-members", func(t *testing.T) {
 		sqlDBRestore1.Exec(t, "RESTORE SYSTEM USERS FROM $1", localFoo+"/3")
 
-		sqlDBRestore1.CheckQueryResults(t, "SELECT * FROM system.users", [][]string{
+		sqlDBRestore1.CheckQueryResults(t, "SELECT username, \"hashedPassword\", \"isRole\" FROM system.users", [][]string{
 			{"admin", "", "true"},
 			{"app", "NULL", "false"},
 			{"app_role", "NULL", "true"},
 			{"root", "", "false"},
 			{"test", "NULL", "false"},
 			{"test_role", "NULL", "true"},
 		})
-		sqlDBRestore1.CheckQueryResults(t, "SELECT * FROM system.role_members", [][]string{
+		sqlDBRestore1.CheckQueryResults(t, "SELECT \"role\", \"member\", \"isAdmin\" FROM system.role_members", [][]string{
 			{"admin", "root", "true"},
 		})
 		sqlDBRestore1.CheckQueryResults(t, "SHOW USERS", [][]string{
@@ -10146,6 +10154,17 @@ func TestBackupLatestInBaseDirectory(t *testing.T) {
 	require.NoError(t, err)
 	r.Close(ctx)
 
+	// Drop the system.role_seq_id so that we can perform the migration.
+	sqlDB.Exec(t, `INSERT INTO system.users VALUES ('node', '', false, 0)`)
+	sqlDB.Exec(t, `GRANT node TO root`)
+	sqlDB.Exec(t, `DROP SEQUENCE system.role_id_seq`)
+	sqlDB.Exec(t, `REVOKE node FROM root`)
+
+	err = tc.Servers[0].DB().Del(ctx, catalogkeys.MakeDescMetadataKey(keys.SystemSQLCodec, keys.RoleIDSequenceID))
+	require.NoError(t, err)
+	err = tc.Servers[0].DB().Del(ctx, keys.SystemSQLCodec.SequenceKey(uint32(keys.RoleIDSequenceID)))
+	require.NoError(t, err)
+
 	// Close the channel so that the cluster version is upgraded.
 	close(disableUpgradeCh)
 	// Check the cluster version is bumped to newVersion.

diff --git a/pkg/ccl/backupccl/full_cluster_backup_restore_test.go b/pkg/ccl/backupccl/full_cluster_backup_restore_test.go
@@ -592,7 +592,7 @@ func TestRestoreFromFullClusterBackup(t *testing.T) {
 
 	t.Run("system tables", func(t *testing.T) {
 		sqlDB.Exec(t, `CREATE DATABASE temp_sys`)
-		sqlDB.Exec(t, `RESTORE system.users FROM $1 WITH into_db='temp_sys'`, localFoo)
+		sqlDB.Exec(t, `RESTORE system.users, system.role_id_seq FROM $1 WITH into_db='temp_sys'`, localFoo)
 		sqlDB.CheckQueryResults(t, "SELECT * FROM temp_sys.users", sqlDB.QueryStr(t, "SELECT * FROM system.users"))
 	})
 }
@@ -678,6 +678,7 @@ func TestClusterRestoreFailCleanup(t *testing.T) {
 				{"external_connections"},
 				{"jobs"},
 				{"locations"},
+				{"role_id_seq"},
 				{"role_members"},
 				{"role_options"},
 				{"scheduled_jobs"},
@@ -770,6 +771,7 @@ func TestClusterRestoreFailCleanup(t *testing.T) {
 				{"external_connections"},
 				{"jobs"},
 				{"locations"},
+				{"role_id_seq"},
 				{"role_members"},
 				{"role_options"},
 				{"scheduled_jobs"},

diff --git a/pkg/ccl/backupccl/system_schema.go b/pkg/ccl/backupccl/system_schema.go
@@ -414,6 +414,9 @@ var systemTableBackupConfiguration = map[string]systemBackupConfiguration{
 	systemschema.SystemExternalConnectionsTable.GetName(): {
 		shouldIncludeInClusterBackup: optInToClusterBackup,
 	},
+	systemschema.RoleIDSequence.GetName(): {
+		shouldIncludeInClusterBackup: optOutOfClusterBackup,
+	},
 }
 
 // GetSystemTablesToIncludeInClusterBackup returns a set of system table names that

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/feature-flags b/pkg/ccl/backupccl/testdata/backup-restore/feature-flags
@@ -81,10 +81,4 @@ RESTORE TABLE d.t FROM 'nodelocal://1/deprecated';
 ----
 NOTICE: The `RESTORE FROM <backup>` syntax will be removed in a future release, please switch over to using `RESTORE FROM <backup> IN <collection>` to restore a particular backup from a collection: https://www.cockroachlabs.com/docs/stable/restore.html#view-the-backup-subdirectories
 
-
-exec-sql
-RESTORE SYSTEM USERS FROM 'nodelocal://1/deprecated';
-----
-NOTICE: The `RESTORE FROM <backup>` syntax will be removed in a future release, please switch over to using `RESTORE FROM <backup> IN <collection>` to restore a particular backup from a collection: https://www.cockroachlabs.com/docs/stable/restore.html#view-the-backup-subdirectories
-
 subtest end
diff --git a/pkg/ccl/spanconfigccl/spanconfigcomparedccl/testdata/basic b/pkg/ccl/spanconfigccl/spanconfigcomparedccl/testdata/basic
@@ -0,0 +1,125 @@
+# Create a database with some tables and set some zone configs; compare how the
+# gossip-backed config subsystem compares to the span configs infrastructure.
+
+reconcile
+----
+
+exec-sql
+CREATE DATABASE db;
+CREATE TABLE db.t1();
+CREATE TABLE db.t2();
+----
+
+exec-sql
+ALTER DATABASE db CONFIGURE ZONE USING num_replicas = 7;
+ALTER TABLE db.t1 CONFIGURE ZONE USING num_voters = 5;
+----
+
+# Both subsystems don't split within the system config span.
+# - The gossip-backed system doesn't because it needs to gossips the entire
+#   range's contents whenever anything in it changes.
+# - The span configs infrastructure doesn't, at least for now, for
+#   inter-operability with the gossip-backed system.
+# That said, the span configs infrastructure is used to drive whether
+# rangefeeds are enabled on the system database tables. It also controls
+# whether strict GC is enforced. For that reason we expect to find different
+# span configs from each ("range system" vs. "database system").
+
+configs version=legacy offset=4 limit=3
+----
+...
+/System/"tse"                              database system (host)
+/Table/SystemConfigSpan/Start              database system (host)
+/Table/11                                  database system (host)
+...
+
+configs version=current offset=4 limit=3
+----
+...
+/System/"tse"                              range system
+/Table/SystemConfigSpan/Start              database system (host)
+/Table/11                                  database system (host)
+...
+
+# Both subsystems observe splits for the tables created above.
+
+configs version=current offset=41
+----
+...
+/Table/46                                  database system (host)
+/Table/47                                  database system (host)
+/Table/48                                  database system (host)
+/Table/50                                  database system (host)
+/Table/106                                 num_replicas=7 num_voters=5
+/Table/107                                 num_replicas=7
+
+configs version=legacy offset=41
+----
+...
+/Table/46                                  database system (host)
+/Table/47                                  database system (host)
+/Table/48                                  database system (host)
+/Table/50                                  range system
+/Table/106                                 num_replicas=7 num_voters=5
+/Table/107                                 num_replicas=7
+
+# Both subsystems differ slightly with respect to exposed configs ("range
+# system" vs. "database system" as described earlier). This only applies to
+# tables in the system database, excluding pseudo table IDs.
+
+diff
+----
+--- gossiped system config span (legacy)
++++ span config infrastructure (current)
+@@ -1,7 +1,7 @@
+-/Min                                       ttl_seconds=3600 ignore_strict_gc=true num_replicas=5 rangefeed_enabled=true
+-/System/NodeLiveness                       ttl_seconds=600 ignore_strict_gc=true num_replicas=5 rangefeed_enabled=true
+-/System/NodeLivenessMax                    database system (host)
+-/System/tsd                                database system (tenant)
+-/System/"tse"                              database system (host)
++/Min                                       ttl_seconds=3600 num_replicas=5
++/System/NodeLiveness                       ttl_seconds=600 num_replicas=5
++/System/NodeLivenessMax                    range system
++/System/tsd                                range default
++/System/"tse"                              range system
+ /Table/SystemConfigSpan/Start              database system (host)
+ /Table/11                                  database system (host)
+@@ -10,11 +10,11 @@
+ /Table/14                                  database system (host)
+ /Table/15                                  database system (host)
+-/Table/16                                  database system (host)
+-/Table/17                                  database system (host)
+-/Table/18                                  database system (host)
++/Table/16                                  range system
++/Table/17                                  range system
++/Table/18                                  range system
+ /Table/19                                  database system (host)
+ /Table/20                                  database system (host)
+ /Table/21                                  database system (host)
+-/Table/22                                  database system (host)
++/Table/22                                  range system
+ /Table/23                                  database system (host)
+ /Table/24                                  database system (host)
+@@ -23,5 +23,5 @@
+ /Table/27                                  ttl_seconds=600 ignore_strict_gc=true num_replicas=5 rangefeed_enabled=true
+ /Table/28                                  database system (host)
+-/Table/29                                  database system (host)
++/Table/29                                  range system
+ /NamespaceTable/30                         database system (host)
+ /NamespaceTable/Max                        database system (host)
+@@ -32,5 +32,5 @@
+ /Table/36                                  database system (host)
+ /Table/37                                  database system (host)
+-/Table/38                                  database system (host)
++/Table/38                                  range system
+ /Table/39                                  database system (host)
+ /Table/40                                  database system (host)
+@@ -43,5 +43,5 @@
+ /Table/47                                  database system (host)
+ /Table/48                                  database system (host)
+-/Table/50                                  range system
++/Table/50                                  database system (host)
+ /Table/106                                 num_replicas=7 num_voters=5
+ /Table/107                                 num_replicas=7
+
+# vim:ft=diff