sql: add user id column to system.users table and perform migration

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-32	set the active cluster version in the format '<major>.<minor>'
+version	version	22.1-40	set the active cluster version in the format '<major>.<minor>'

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-32</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-40</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
 </tbody>
 </table>

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

pkg/ccl/backupccl/BUILD.bazel

@@ -78,6 +78,7 @@ go_library(
         "//pkg/sql/catalog/colinfo",
         "//pkg/sql/catalog/dbdesc",
         "//pkg/sql/catalog/descbuilder",
+        "//pkg/sql/catalog/descidgen",
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/descs",
         "//pkg/sql/catalog/ingesting",

pkg/ccl/backupccl/backup_test.go

@@ -9574,7 +9574,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"},
@@ -9607,18 +9607,17 @@ func TestBackupRestoreSystemUsers(t *testing.T) {
 	defer cleanupEmptyCluster1()
 	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{
-			{"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, "SELECT username, \"hashedPassword\", \"isRole\", \"user_id\" FROM system.users", [][]string{
+			{"admin", "", "true", "2"},
+			{"app", "NULL", "false", "100"},
+			{"app_role", "NULL", "true", "101"},
+			{"root", "", "false", "1"},
+			{"test", "NULL", "false", "102"},
+			{"test_role", "NULL", "true", "103"},
+		})
 		sqlDBRestore1.CheckQueryResults(t, "SHOW USERS", [][]string{
 			{"admin", "", "{}"},
 			{"app", "", "{}"},
@@ -9628,6 +9627,35 @@ func TestBackupRestoreSystemUsers(t *testing.T) {
 			{"test_role", "", "{}"},
 		})
 	})
+	_, sqlDBRestore2, cleanupEmptyCluster2 := backupRestoreTestSetupEmpty(t, singleNode, tempDir, InitManualReplication, base.TestClusterArgs{})
+	defer cleanupEmptyCluster2()
+	t.Run("restore-from-backup-with-existing-user", func(t *testing.T) {
+		// Create testuser and verify that the system user ids are
+		// allocated properly in the restore.
+		sqlDBRestore2.Exec(t, "CREATE USER testuser")
+		sqlDBRestore2.Exec(t, "RESTORE SYSTEM USERS FROM $1", localFoo+"/3")
+		sqlDBRestore2.CheckQueryResults(t, "SELECT \"role\", \"member\", \"isAdmin\" FROM system.role_members", [][]string{
+			{"admin", "root", "true"},
+		})
+		sqlDBRestore2.CheckQueryResults(t, "SELECT username, \"hashedPassword\", \"isRole\", \"user_id\" FROM system.users", [][]string{
+			{"admin", "", "true", "2"},
+			{"app", "NULL", "false", "101"},
+			{"app_role", "NULL", "true", "102"},
+			{"root", "", "false", "1"},
+			{"test", "NULL", "false", "103"},
+			{"test_role", "NULL", "true", "104"},
+			{"testuser", "NULL", "false", "100"},
+		})
+		sqlDBRestore2.CheckQueryResults(t, "SHOW USERS", [][]string{
+			{"admin", "", "{}"},
+			{"app", "", "{}"},
+			{"app_role", "", "{}"},
+			{"root", "", "{admin}"},
+			{"test", "", "{}"},
+			{"test_role", "", "{}"},
+			{"testuser", "", "{}"},
+		})
+	})
 }
 
 // TestUserfileNormalizationIncrementalShowBackup tests to see that file

pkg/ccl/backupccl/full_cluster_backup_restore_test.go

@@ -557,7 +557,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"))
 	})
 }
@@ -642,6 +642,7 @@ func TestClusterRestoreFailCleanup(t *testing.T) {
 				{"database_role_settings"},
 				{"external_connections"},
 				{"locations"},
+				{"role_id_seq"},
 				{"role_members"},
 				{"role_options"},
 				{"scheduled_jobs"},
@@ -733,6 +734,7 @@ func TestClusterRestoreFailCleanup(t *testing.T) {
 				{"database_role_settings"},
 				{"external_connections"},
 				{"locations"},
+				{"role_id_seq"},
 				{"role_members"},
 				{"role_options"},
 				{"scheduled_jobs"},
@@ -1110,3 +1112,51 @@ DROP DATABASE defaultdb;
 		{fmt.Sprint(parentID), fmt.Sprint(parentSchemaID), name, fmt.Sprint(ID)},
 	})
 }
+
+func TestFullClusterRestoreWithUserIDs(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	params := base.TestClusterArgs{
+		ServerArgs: base.TestServerArgs{
+			Knobs: base.TestingKnobs{
+				JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(),
+			},
+		},
+	}
+	const numAccounts = 10
+	_, sqlDB, tempDir, cleanupFn := backupRestoreTestSetupWithParams(t, singleNode, numAccounts, InitManualReplication, params)
+	_, sqlDBRestore, cleanupEmptyCluster := backupRestoreTestSetupEmpty(t, singleNode, tempDir, InitManualReplication, params)
+	defer cleanupFn()
+	defer cleanupEmptyCluster()
+
+	sqlDB.Exec(t, `CREATE USER test1`)
+	sqlDB.Exec(t, `CREATE USER test2`)
+	sqlDB.Exec(t, `BACKUP TO $1`, localFoo)
+
+	sqlDB.CheckQueryResults(t, `SELECT * FROM system.users ORDER BY user_id`, [][]string{
+		{"root", "", "false", "1"},
+		{"admin", "", "true", "2"},
+		{"test1", "NULL", "false", "100"},
+		{"test2", "NULL", "false", "101"},
+	})
+	// Ensure that the new backup succeeds.
+	sqlDBRestore.Exec(t, `RESTORE FROM $1`, localFoo)
+
+	sqlDBRestore.CheckQueryResults(t, `SELECT * FROM system.users ORDER BY user_id`, [][]string{
+		{"root", "", "false", "1"},
+		{"admin", "", "true", "2"},
+		{"test1", "NULL", "false", "100"},
+		{"test2", "NULL", "false", "101"},
+	})
+
+	sqlDBRestore.Exec(t, `CREATE USER test3`)
+
+	sqlDBRestore.CheckQueryResults(t, `SELECT * FROM system.users ORDER BY user_id`, [][]string{
+		{"root", "", "false", "1"},
+		{"admin", "", "true", "2"},
+		{"test1", "NULL", "false", "100"},
+		{"test2", "NULL", "false", "101"},
+		{"test3", "NULL", "false", "102"},
+	})
+}

pkg/ccl/backupccl/restore_job.go

@@ -37,6 +37,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/ingesting"
@@ -2400,11 +2401,25 @@ func (r *restoreResumer) restoreSystemUsers(
 		}
 
 		insertUser := `INSERT INTO system.users ("username", "hashedPassword", "isRole") VALUES ($1, $2, $3)`
+		if r.execCfg.Settings.Version.IsActive(ctx, clusterversion.AddSystemUserIDColumn) {
+			insertUser = `INSERT INTO system.users ("username", "hashedPassword", "isRole", "user_id") VALUES ($1, $2, $3, $4)`
+		}
 		newUsernames := make(map[string]bool)
+		args := make([]interface{}, 4)
 		for _, user := range users {
 			newUsernames[user[0].String()] = true
+			args[0] = user[0]
+			args[1] = user[1]
+			args[2] = user[2]
+			if r.execCfg.Settings.Version.IsActive(ctx, clusterversion.AddSystemUserIDColumn) {
+				id, err := descidgen.GenerateUniqueRoleID(ctx, r.execCfg.DB, r.execCfg.Codec)
+				if err != nil {
+					return err
+				}
+				args[3] = id
+			}
 			if _, err = executor.Exec(ctx, "insert-non-existent-users", txn, insertUser,
-				user[0], user[1], user[2]); err != nil {
+				args...); err != nil {
 				return err
 			}
 		}

pkg/ccl/backupccl/restore_old_versions_test.go

@@ -74,6 +74,7 @@ func TestRestoreOldVersions(t *testing.T) {
 		privilegeDirs               = testdataBase + "/privileges"
 		multiRegionDirs             = testdataBase + "/multi-region"
 		publicSchemaDirs            = testdataBase + "/public-schema-remap"
+		systemUsersDirs             = testdataBase + "/system-users-restore"
 	)
 
 	t.Run("table-restore", func(t *testing.T) {
@@ -300,6 +301,28 @@ ORDER BY object_type, object_name`, [][]string{
 			t.Run(dir.Name(), restoreSyntheticPublicSchemaNamespaceEntryCleanupOnFail(exportDir))
 		}
 	})
+
+	t.Run("system-users-restore", func(t *testing.T) {
+		dirs, err := ioutil.ReadDir(systemUsersDirs)
+		require.NoError(t, err)
+		for _, dir := range dirs {
+			require.True(t, dir.IsDir())
+			exportDir, err := filepath.Abs(filepath.Join(systemUsersDirs, dir.Name()))
+			require.NoError(t, err)
+			t.Run(dir.Name(), restoreSystemUsersWithoutIDs(exportDir))
+		}
+	})
+
+	t.Run("full-cluster-restore-users-without-ids", func(t *testing.T) {
+		dirs, err := ioutil.ReadDir(systemUsersDirs)
+		require.NoError(t, err)
+		for _, dir := range dirs {
+			require.True(t, dir.IsDir())
+			exportDir, err := filepath.Abs(filepath.Join(systemUsersDirs, dir.Name()))
+			require.NoError(t, err)
+			t.Run(dir.Name(), fullClusterRestoreUsersWithoutIDs(exportDir))
+		}
+	})
 }
 
 func restoreOldVersionTestWithInterleave(exportDir string) func(t *testing.T) {
@@ -1112,3 +1135,119 @@ func restoreSyntheticPublicSchemaNamespaceEntryCleanupOnFail(exportDir string) f
 		sqlDB.CheckQueryResults(t, `SELECT id FROM system.namespace WHERE name = 'public' AND id=29 AND "parentID"!=1`, [][]string{})
 	}
 }
+
+func fullClusterRestoreUsersWithoutIDs(exportDir string) func(t *testing.T) {
+	return func(t *testing.T) {
+		const numAccounts = 1000
+		_, _, tmpDir, cleanupFn := backupRestoreTestSetup(t, multiNode, numAccounts, InitManualReplication)
+		defer cleanupFn()
+
+		_, sqlDB, cleanup := backupRestoreTestSetupEmpty(t, singleNode, tmpDir,
+			InitManualReplication, base.TestClusterArgs{
+				ServerArgs: base.TestServerArgs{
+					Knobs: base.TestingKnobs{
+						JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(),
+					},
+				}})
+		defer cleanup()
+		err := os.Symlink(exportDir, filepath.Join(tmpDir, "foo"))
+		require.NoError(t, err)
+
+		sqlDB.Exec(t, fmt.Sprintf("RESTORE FROM '%s'", localFoo))
+
+		sqlDB.CheckQueryResults(t, `SELECT username, "hashedPassword", "isRole", user_id FROM system.users`, [][]string{
+			{"admin", "", "true", "2"},
+			{"root", "", "false", "1"},
+			{"testrole", "NULL", "true", "100"},
+			{"testuser", "NULL", "false", "101"},
+			{"testuser2", "NULL", "false", "102"},
+			{"testuser3", "NULL", "false", "103"},
+		})
+
+		// Verify that the next user we create uses the next biggest ID.
+		sqlDB.Exec(t, "CREATE USER testuser4")
+
+		sqlDB.CheckQueryResults(t, `SELECT username, "hashedPassword", "isRole", user_id FROM system.users`, [][]string{
+			{"admin", "", "true", "2"},
+			{"root", "", "false", "1"},
+			{"testrole", "NULL", "true", "100"},
+			{"testuser", "NULL", "false", "101"},
+			{"testuser2", "NULL", "false", "102"},
+			{"testuser3", "NULL", "false", "103"},
+			{"testuser4", "NULL", "false", "104"},
+		})
+	}
+}
+
+func restoreSystemUsersWithoutIDs(exportDir string) func(t *testing.T) {
+	return func(t *testing.T) {
+		const numAccounts = 1000
+		_, _, tmpDir, cleanupFn := backupRestoreTestSetup(t, multiNode, numAccounts, InitManualReplication)
+		defer cleanupFn()
+
+		_, sqlDB, cleanup := backupRestoreTestSetupEmpty(t, singleNode, tmpDir,
+			InitManualReplication, base.TestClusterArgs{
+				ServerArgs: base.TestServerArgs{
+					Knobs: base.TestingKnobs{
+						JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(),
+					},
+				}})
+		defer cleanup()
+		err := os.Symlink(exportDir, filepath.Join(tmpDir, "foo"))
+		require.NoError(t, err)
+
+		sqlDB.Exec(t, fmt.Sprintf("RESTORE SYSTEM USERS FROM '%s'", localFoo))
+
+		sqlDB.CheckQueryResults(t, `SELECT username, "hashedPassword", "isRole", user_id FROM system.users`, [][]string{
+			{"admin", "", "true", "2"},
+			{"root", "", "false", "1"},
+			{"testrole", "NULL", "true", "100"},
+			{"testuser", "NULL", "false", "101"},
+			{"testuser2", "NULL", "false", "102"},
+			{"testuser3", "NULL", "false", "103"},
+		})
+
+		// Verify that the next user we create uses the next biggest ID.
+		sqlDB.Exec(t, "CREATE USER testuser4")
+
+		sqlDB.CheckQueryResults(t, `SELECT username, "hashedPassword", "isRole", user_id FROM system.users`, [][]string{
+			{"admin", "", "true", "2"},
+			{"root", "", "false", "1"},
+			{"testrole", "NULL", "true", "100"},
+			{"testuser", "NULL", "false", "101"},
+			{"testuser2", "NULL", "false", "102"},
+			{"testuser3", "NULL", "false", "103"},
+			{"testuser4", "NULL", "false", "104"},
+		})
+
+		// Drop some users and try restoring again.
+		sqlDB.Exec(t, "DROP ROLE testrole")
+		sqlDB.Exec(t, "DROP ROLE testuser2")
+		sqlDB.Exec(t, "DROP ROLE testuser3")
+		sqlDB.Exec(t, "DROP ROLE testuser4")
+
+		sqlDB.Exec(t, fmt.Sprintf("RESTORE SYSTEM USERS FROM '%s'", localFoo))
+
+		// testrole, testuser2, testuser3 should be reassigned higher ids.
+		sqlDB.CheckQueryResults(t, `SELECT username, "hashedPassword", "isRole", user_id FROM system.users`, [][]string{
+			{"admin", "", "true", "2"},
+			{"root", "", "false", "1"},
+			{"testrole", "NULL", "true", "105"},
+			{"testuser", "NULL", "false", "101"},
+			{"testuser2", "NULL", "false", "106"},
+			{"testuser3", "NULL", "false", "107"},
+		})
+
+		// Verify that the next user we create uses the next biggest ID.
+		sqlDB.Exec(t, "CREATE USER testuser4")
+		sqlDB.CheckQueryResults(t, `SELECT username, "hashedPassword", "isRole", user_id FROM system.users`, [][]string{
+			{"admin", "", "true", "2"},
+			{"root", "", "false", "1"},
+			{"testrole", "NULL", "true", "105"},
+			{"testuser", "NULL", "false", "101"},
+			{"testuser2", "NULL", "false", "106"},
+			{"testuser3", "NULL", "false", "107"},
+			{"testuser4", "NULL", "false", "108"},
+		})
+	}
+}