sql: add role_id, member_id columns to system.role_members

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

@@ -289,4 +289,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-58	set the active cluster version in the format '<major>.<minor>'
+version	version	22.1-64	set the active cluster version in the format '<major>.<minor>'

docs/generated/settings/settings.html

@@ -220,6 +220,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-58</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-64</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
 </tbody>
 </table>

pkg/bench/rttanalysis/testdata/benchmark_expectations

@@ -48,10 +48,10 @@ exp,benchmark
 19,DropView/drop_2_views
 19,DropView/drop_3_views
 14,Grant/grant_all_on_1_table
-14,Grant/grant_all_on_2_tables
-14,Grant/grant_all_on_3_tables
-11,GrantRole/grant_1_role
-13,GrantRole/grant_2_roles
+15,Grant/grant_all_on_2_tables
+15,Grant/grant_all_on_3_tables
+13,GrantRole/grant_1_role
+17,GrantRole/grant_2_roles
 2,ORMQueries/activerecord_type_introspection_query
 2,ORMQueries/django_table_introspection_1_table
 2,ORMQueries/django_table_introspection_4_tables

pkg/ccl/backupccl/backup_test.go

@@ -9459,11 +9459,11 @@ func TestBackupRestoreSystemUsers(t *testing.T) {
 			{"test", "NULL", "false"},
 			{"test_role", "NULL", "true"},
 		})
-		sqlDBRestore.CheckQueryResults(t, "SELECT * FROM system.role_members", [][]string{
-			{"admin", "app", "false"},
-			{"admin", "root", "true"},
-			{"app_role", "app", "false"},
-			{"app_role", "test_role", "false"},
+		sqlDBRestore.CheckQueryResults(t, `SELECT role, member, "isAdmin", role_id, member_id FROM system.role_members`, [][]string{
+			{"admin", "app", "false", "2", "100"},
+			{"admin", "root", "true", "2", "1"},
+			{"app_role", "app", "false", "102", "100"},
+			{"app_role", "test_role", "false", "102", "103"},
 		})
 		sqlDBRestore.CheckQueryResults(t, "SHOW USERS", [][]string{
 			{"admin", "", "{}"},

pkg/ccl/backupccl/restore_job.go

@@ -2442,6 +2442,14 @@ func (r *restoreResumer) restoreSystemUsers(
 			return nil
 		}
 
+		hasIDColumnsQuery := fmt.Sprintf(
+			`SELECT EXISTS (SELECT 1 FROM [SHOW COLUMNS FROM %s] WHERE column_name = 'role_id')`, "crdb_temp_system.role_members")
+		row, err := executor.QueryRow(ctx, "has-id-column", txn, hasIDColumnsQuery)
+		if err != nil {
+			return err
+		}
+		backupSystemRoleMembersHsIDColumns := bool(tree.MustBeDBool(row[0]))
+
 		selectNonExistentRoleMembers := "SELECT * FROM crdb_temp_system.role_members temp_rm WHERE " +
 			"NOT EXISTS (SELECT * FROM system.role_members rm WHERE temp_rm.role = rm.role AND temp_rm.member = rm.member)"
 		roleMembers, err := executor.QueryBuffered(ctx, "get-role-members",
@@ -2450,12 +2458,43 @@ func (r *restoreResumer) restoreSystemUsers(
 			return err
 		}
 
+		roleMembersHasIDColumn := r.execCfg.Settings.Version.IsActive(ctx, clusterversion.RoleMembersTableHasIDColumns)
 		insertRoleMember := `INSERT INTO system.role_members ("role", "member", "isAdmin") VALUES ($1, $2, $3)`
+		if roleMembersHasIDColumn {
+			insertRoleMember = `INSERT INTO system.role_members ("role", "member", "isAdmin", "role_id", "member_id") VALUES ($1, $2, $3, $4, $5)`
+		}
+
+		var qargs []interface{}
+		if roleMembersHasIDColumn {
+			qargs = make([]interface{}, 5)
+		} else {
+			qargs = make([]interface{}, 3)
+		}
+
 		for _, roleMember := range roleMembers {
 			// Only grant roles to users that don't currently exist, i.e., new users we just added
 			if _, ok := newUsernames[roleMember[1].String()]; ok {
+				qargs[0], qargs[1], qargs[2] = roleMember[0], roleMember[1], roleMember[2]
+
+				if roleMembersHasIDColumn && !backupSystemRoleMembersHsIDColumns {
+					// Populate the IDs by getting it from the system.users table.
+					roleID, err := getIDForUser(ctx, tree.MustBeDString(roleMember[0]), executor, txn)
+					if err != nil {
+						return err
+					}
+					memberID, err := getIDForUser(ctx, tree.MustBeDString(roleMember[1]), executor, txn)
+					if err != nil {
+						return err
+					}
+					qargs[3] = roleID
+					qargs[4] = memberID
+				} else {
+					qargs[3] = roleMember[3]
+					qargs[4] = roleMember[4]
+				}
+
 				if _, err = executor.Exec(ctx, "insert-non-existent-role-members", txn, insertRoleMember,
-					roleMember[0], roleMember[1], roleMember[2]); err != nil {
+					qargs...); err != nil {
 					return err
 				}
 			}

pkg/ccl/backupccl/restore_old_versions_test.go

@@ -1164,6 +1164,17 @@ func fullClusterRestoreUsersWithoutIDs(exportDir string) func(t *testing.T) {
 			{"testuser4", "NULL", "false", "104"},
 		})
 
+		sqlDB.CheckQueryResults(t, `SELECT role, member "isAdmin", role_id, member_id FROM system.role_members`, [][]string{
+			{"admin", "root", "2", "1"},
+			{"admin", "testuser4", "2", "104"},
+			{"testrole", "testuser", "100", "101"},
+			{"testrole", "testuser2", "100", "102"},
+			{"testuser", "root", "101", "1"},
+			{"testuser", "testuser2", "101", "102"},
+			{"testuser", "testuser3", "101", "103"},
+			{"testuser4", "testuser2", "104", "102"},
+		})
+
 		sqlDB.CheckQueryResults(t, `SELECT * FROM system.role_options`, [][]string{
 			{"testrole", "NOLOGIN", "NULL", "100"},
 			{"testuser", "CREATEROLE", "NULL", "101"},
@@ -1194,6 +1205,19 @@ func fullClusterRestoreUsersWithoutIDs(exportDir string) func(t *testing.T) {
 			{"testuser4", "NULL", "false", "104"},
 			{"testuser5", "NULL", "false", "105"},
 		})
+
+		sqlDB.Exec(t, "GRANT testuser5 TO testuser")
+		sqlDB.CheckQueryResults(t, `SELECT role, member "isAdmin", role_id, member_id FROM system.role_members`, [][]string{
+			{"admin", "root", "2", "1"},
+			{"admin", "testuser4", "2", "104"},
+			{"testrole", "testuser", "100", "101"},
+			{"testrole", "testuser2", "100", "102"},
+			{"testuser", "root", "101", "1"},
+			{"testuser", "testuser2", "101", "102"},
+			{"testuser", "testuser3", "101", "103"},
+			{"testuser4", "testuser2", "104", "102"},
+			{"testuser5", "testuser", "105", "101"},
+		})
 	}
 }
 
@@ -1226,6 +1250,20 @@ func restoreSystemUsersWithoutIDs(exportDir string) func(t *testing.T) {
 			{"testuser4", "NULL", "false", "104"},
 		})
 
+		// Note: in the backup, we grant `testuser` to `root`.
+		// However, we don't restore that row since we only restore
+		// memberships for users that did not previously exist in
+		// the cluster.
+		sqlDB.CheckQueryResults(t, `SELECT role, member "isAdmin", role_id, member_id FROM system.role_members`, [][]string{
+			{"admin", "root", "2", "1"},
+			{"admin", "testuser4", "2", "104"},
+			{"testrole", "testuser", "100", "101"},
+			{"testrole", "testuser2", "100", "102"},
+			{"testuser", "testuser2", "101", "102"},
+			{"testuser", "testuser3", "101", "103"},
+			{"testuser4", "testuser2", "104", "102"},
+		})
+
 		// Verify that the next user we create uses the next biggest ID.
 		sqlDB.Exec(t, "CREATE USER testuser5")
 

pkg/ccl/backupccl/system_schema.go

@@ -296,18 +296,9 @@ func roleOptionsRestoreFunc(
 		option := tree.MustBeDString(it.Cur()[1])
 		val := it.Cur()[2]
 
-		var id int64
-		if username == "root" {
-			id = 1
-		} else if username == "admin" {
-			id = 2
-		} else {
-			row, err := executor.QueryRow(ctx, `get-user-id`, txn, `SELECT user_id FROM system.users WHERE username = $1`, username)
-			if err != nil {
-				return err
-			}
-			oid := tree.MustBeDOid(row[0])
-			id = int64(oid.Oid)
+		id, err := getIDForUser(ctx, username, executor, txn)
+		if err != nil {
+			return err
 		}
 
 		restoreQuery := fmt.Sprintf("INSERT INTO system.%s VALUES ($1, $2, $3, $4)",
@@ -320,6 +311,97 @@ func roleOptionsRestoreFunc(
 	return nil
 }
 
+func roleMembersRestoreFunc(
+	ctx context.Context,
+	execCfg *sql.ExecutorConfig,
+	txn *kv.Txn,
+	systemTableName, tempTableName string,
+) error {
+	if !execCfg.Settings.Version.IsActive(ctx, clusterversion.RoleMembersTableHasIDColumns) {
+		return defaultSystemTableRestoreFunc(
+			ctx, execCfg, txn, systemTableName, tempTableName,
+		)
+	}
+
+	executor := execCfg.InternalExecutor
+	hasIDColumnsQuery := fmt.Sprintf(
+		`SELECT EXISTS (SELECT 1 FROM [SHOW COLUMNS FROM %s] WHERE column_name = 'role_id')`, tempTableName)
+	row, err := executor.QueryRow(ctx, "has-id-column", txn, hasIDColumnsQuery)
+	if err != nil {
+		return err
+	}
+	hasIDColumn := tree.MustBeDBool(row[0])
+	if hasIDColumn {
+		return defaultSystemTableRestoreFunc(
+			ctx, execCfg, txn, systemTableName, tempTableName,
+		)
+	}
+
+	deleteQuery := fmt.Sprintf("DELETE FROM system.%s WHERE true", systemTableName)
+	opName := systemTableName + "-data-deletion"
+	log.Eventf(ctx, "clearing data from system table %s with query %q",
+		systemTableName, deleteQuery)
+
+	_, err = executor.Exec(ctx, opName, txn, deleteQuery)
+	if err != nil {
+		return errors.Wrapf(err, "deleting data from system.%s", systemTableName)
+	}
+
+	it, err := executor.QueryIteratorEx(ctx, "query-system-role-members-in-backup",
+		txn, sessiondata.NodeUserSessionDataOverride,
+		fmt.Sprintf(`SELECT * FROM %s`, tempTableName))
+	if err != nil {
+		return err
+	}
+
+	for {
+		ok, err := it.Next(ctx)
+		if err != nil {
+			return err
+		}
+		if !ok {
+			break
+		}
+
+		roleName := tree.MustBeDString(it.Cur()[0])
+		memberName := tree.MustBeDString(it.Cur()[1])
+		isAdmin := tree.MustBeDBool(it.Cur()[2])
+
+		roleID, err := getIDForUser(ctx, roleName, executor, txn)
+		if err != nil {
+			return err
+		}
+		memberID, err := getIDForUser(ctx, memberName, executor, txn)
+		if err != nil {
+			return err
+		}
+
+		restoreQuery := fmt.Sprintf("INSERT INTO system.%s VALUES ($1, $2, $3, $4, $5)",
+			systemTableName)
+		opName = systemTableName + "-data-insert"
+		if _, err := executor.Exec(ctx, opName, txn, restoreQuery, roleName, memberName, isAdmin, roleID, memberID); err != nil {
+			return errors.Wrapf(err, "inserting data to system.%s", systemTableName)
+		}
+	}
+	return nil
+}
+
+func getIDForUser(
+	ctx context.Context, name tree.DString, executor *sql.InternalExecutor, txn *kv.Txn,
+) (int64, error) {
+	if name == "root" {
+		return 1, nil
+	} else if name == "admin" {
+		return 2, nil
+	} else {
+		row, err := executor.QueryRow(ctx, `get-user-id`, txn, `SELECT user_id FROM system.users WHERE username = $1`, name)
+		if err != nil {
+			return 0, err
+		}
+		return int64(tree.MustBeDOid(row[0]).Oid), nil
+	}
+}
+
 // When restoring the settings table, we want to make sure to not override the
 // version.
 func settingsRestoreFunc(
@@ -400,6 +482,8 @@ var systemTableBackupConfiguration = map[string]systemBackupConfiguration{
 	},
 	systemschema.RoleMembersTable.GetName(): {
 		shouldIncludeInClusterBackup: optInToClusterBackup, // No desc ID columns.
+		customRestoreFunc:            roleMembersRestoreFunc,
+		restoreInOrder:               1, // Restore after system.users.
 	},
 	systemschema.RoleOptionsTable.GetName(): {
 		shouldIncludeInClusterBackup: optInToClusterBackup, // No desc ID columns.

pkg/clusterversion/cockroach_versions.go

@@ -320,6 +320,16 @@ const (
 	// WaitedForDelRangeInGCJob corresponds to the migration which waits for
 	// the GC jobs to adopt the use of DelRange with tombstones.
 	WaitedForDelRangeInGCJob
+	// RoleMembersTableHasIDColumns is the version where the role
+	// members table has ids.
+	RoleMembersTableHasIDColumns
+	// RoleMembersIDColumnsAreBackfilled is the version where ids in the
+	// role options table are backfilled.
+	RoleMembersIDColumnsAreBackfilled
+	// SetRoleMembersUserIDColumnsNotNull is the version where the role
+	// members table id columns cannot be null. This is the final step
+	// of the system.role_members table migration.
+	SetRoleMembersUserIDColumnsNotNull
 
 	// *************************************************
 	// Step (1): Add new versions here.
@@ -542,6 +552,18 @@ var versionsSingleton = keyedVersions{
 		Key:     WaitedForDelRangeInGCJob,
 		Version: roachpb.Version{Major: 22, Minor: 1, Internal: 58},
 	},
+	{
+		Key:     RoleMembersTableHasIDColumns,
+		Version: roachpb.Version{Major: 22, Minor: 1, Internal: 60},
+	},
+	{
+		Key:     RoleMembersIDColumnsAreBackfilled,
+		Version: roachpb.Version{Major: 22, Minor: 1, Internal: 62},
+	},
+	{
+		Key:     SetRoleMembersUserIDColumnsNotNull,
+		Version: roachpb.Version{Major: 22, Minor: 1, Internal: 64},
+	},
 	// *************************************************
 	// Step (2): Add new versions here.
 	// Do not add new versions to a patch release.

pkg/server/testserver_http.go

@@ -155,7 +155,7 @@ func (ts *httpTestServer) createAuthUser(userName username.SQLUsername, isAdmin
 		if _, err := ts.t.sqlServer.internalExecutor.ExecEx(context.TODO(),
 			"grant-admin", nil,
 			sessiondata.InternalExecutorOverride{User: username.RootUserName()},
-			"INSERT INTO system.role_members (role, member, \"isAdmin\") VALUES ('admin', $1, true)", userName.Normalized(),
+			fmt.Sprintf(`GRANT admin TO %s`, userName.Normalized()),
 		); err != nil {
 			return err
 		}