diff --git a/src/adapter/src/catalog.rs b/src/adapter/src/catalog.rs index 4daef311338f..0da27071c6fe 100644 --- a/src/adapter/src/catalog.rs +++ b/src/adapter/src/catalog.rs @@ -53,7 +53,7 @@ use mz_sql::catalog::{ }; use mz_sql::names::{ Aug, DatabaseId, FullObjectName, ObjectQualifiers, PartialObjectName, QualifiedObjectName, - QualifiedSchemaName, RawDatabaseSpecifier, ResolvedDatabaseSpecifier, SchemaId, + QualifiedSchemaName, RawDatabaseSpecifier, ResolvedDatabaseSpecifier, RoleId, SchemaId, SchemaSpecifier, }; use mz_sql::plan::{ @@ -71,7 +71,7 @@ use mz_transform::Optimizer; use crate::catalog::builtin::{ Builtin, BuiltinLog, BuiltinStorageCollection, BuiltinTable, BuiltinType, Fingerprint, - BUILTINS, BUILTIN_ROLES, INFORMATION_SCHEMA, MZ_CATALOG_SCHEMA, MZ_INTERNAL_SCHEMA, + BUILTINS, BUILTIN_ROLE_PREFIXES, INFORMATION_SCHEMA, MZ_CATALOG_SCHEMA, MZ_INTERNAL_SCHEMA, MZ_TEMP_SCHEMA, PG_CATALOG_SCHEMA, }; pub use crate::catalog::builtin_table_updates::BuiltinTableUpdate; @@ -1169,7 +1169,7 @@ pub struct Schema { #[derive(Debug, Serialize, Clone)] pub struct Role { pub name: String, - pub id: u64, + pub id: RoleId, #[serde(skip)] pub oid: u32, } @@ -1830,8 +1830,7 @@ impl Catalog { } let roles = catalog.storage().await.load_roles().await?; - let builtin_roles = BUILTIN_ROLES.iter().map(|b| (b.id, b.name.to_owned())); - for (id, name) in roles.into_iter().chain(builtin_roles) { + for (id, name) in roles { let oid = catalog.allocate_oid().await?; catalog.state.roles.insert( name.clone(), @@ -3180,7 +3179,7 @@ impl Catalog { schema_name: String, }, CreateRole { - id: u64, + id: RoleId, oid: u32, name: String, }, @@ -3329,7 +3328,7 @@ impl Catalog { ))); } vec![Action::CreateRole { - id: tx.insert_role(&name)?, + id: tx.insert_user_role(&name)?, oid, name, }] @@ -3481,6 +3480,11 @@ impl Catalog { }] } Op::DropRole { name } => { + if is_reserved_name(&name) { + return Err(AdapterError::Catalog(Error::new( + ErrorKind::ReservedRoleName(name), + ))); + } tx.remove_role(&name)?; builtin_table_updates.push(self.state.pack_role_update(&name, -1)); vec![Action::DropRole { name }] @@ -4253,7 +4257,9 @@ impl Catalog { } fn is_reserved_name(name: &str) -> bool { - name.starts_with("mz_") || name.starts_with("pg_") + BUILTIN_ROLE_PREFIXES + .iter() + .any(|prefix| name.starts_with(prefix)) } #[derive(Debug, Clone)] @@ -4762,7 +4768,7 @@ impl mz_sql::catalog::CatalogRole for Role { &self.name } - fn id(&self) -> u64 { + fn id(&self) -> RoleId { self.id } } diff --git a/src/adapter/src/catalog/builtin.rs b/src/adapter/src/catalog/builtin.rs index 4a15fed5c3c9..85dbe12e7123 100644 --- a/src/adapter/src/catalog/builtin.rs +++ b/src/adapter/src/catalog/builtin.rs @@ -112,9 +112,13 @@ pub struct BuiltinFunc { pub inner: &'static mz_sql::func::Func, } +pub static BUILTIN_ROLE_PREFIXES: Lazy> = Lazy::new(|| vec!["mz_", "pg_"]); + pub struct BuiltinRole { + /// Name of the builtin role. + /// + /// IMPORTANT: Must start with a prefix from [`BUILTIN_ROLE_PREFIXES`]. pub name: &'static str, - pub id: u64, } pub trait Fingerprint { @@ -1134,7 +1138,7 @@ pub static MZ_ROLES: Lazy = Lazy::new(|| BuiltinTable { name: "mz_roles", schema: MZ_CATALOG_SCHEMA, desc: RelationDesc::empty() - .with_column("id", ScalarType::Int64.nullable(false)) + .with_column("id", ScalarType::String.nullable(false)) .with_column("oid", ScalarType::Oid.nullable(false)) .with_column("name", ScalarType::String.nullable(false)), }); @@ -2101,10 +2105,7 @@ AS SELECT FROM mz_catalog.mz_roles r", }; -pub const MZ_SYSTEM: BuiltinRole = BuiltinRole { - name: "mz_system", - id: 0, -}; +pub const MZ_SYSTEM: BuiltinRole = BuiltinRole { name: "mz_system" }; pub static BUILTINS_STATIC: Lazy>> = Lazy::new(|| { let mut builtins = vec![ @@ -2293,6 +2294,7 @@ pub static BUILTIN_ROLES: Lazy> = Lazy::new(|| vec![MZ_SYSTEM]) #[allow(non_snake_case)] pub mod BUILTINS { use super::*; + pub fn logs() -> impl Iterator { BUILTINS_STATIC.iter().filter_map(|b| match b { Builtin::Log(log) => Some(*log), @@ -2327,15 +2329,16 @@ mod tests { use std::collections::{HashMap, HashSet}; use std::env; - use mz_ore::now::NOW_ZERO; use tokio_postgres::NoTls; - use crate::catalog::{Catalog, CatalogItem, SYSTEM_CONN_ID}; + use mz_ore::now::NOW_ZERO; use mz_ore::task; use mz_pgrepr::oid::{FIRST_MATERIALIZE_OID, FIRST_UNPINNED_OID}; use mz_sql::catalog::{CatalogSchema, SessionCatalog}; use mz_sql::names::{PartialObjectName, ResolvedDatabaseSpecifier}; + use crate::catalog::{Catalog, CatalogItem, SYSTEM_CONN_ID}; + use super::*; // Connect to a running Postgres server and verify that our builtin diff --git a/src/adapter/src/catalog/builtin_table_updates.rs b/src/adapter/src/catalog/builtin_table_updates.rs index 534012a1be64..cb79b4df8bbb 100644 --- a/src/adapter/src/catalog/builtin_table_updates.rs +++ b/src/adapter/src/catalog/builtin_table_updates.rs @@ -96,8 +96,7 @@ impl CatalogState { BuiltinTableUpdate { id: self.resolve_builtin_table(&MZ_ROLES), row: Row::pack_slice(&[ - // TODO(jkosh44) when Uint64 is supported change below to Datum::Uint64 - Datum::Int64(role.id as i64), + Datum::String(&role.id.to_string()), Datum::UInt32(role.oid), Datum::String(&name), ]), diff --git a/src/adapter/src/catalog/storage.rs b/src/adapter/src/catalog/storage.rs index d5d45e879e5a..3b6ede448dfd 100644 --- a/src/adapter/src/catalog/storage.rs +++ b/src/adapter/src/catalog/storage.rs @@ -7,9 +7,10 @@ // the Business Source License, use of this software will be governed // by the Apache License, Version 2.0. -use std::collections::{BTreeMap, HashMap}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::hash::Hash; use std::iter::once; +use std::str::FromStr; use bytes::BufMut; use itertools::{max, Itertools}; @@ -18,7 +19,6 @@ use serde_json::json; use timely::progress::Timestamp; use uuid::Uuid; -use crate::catalog; use mz_audit_log::{VersionedEvent, VersionedStorageMetrics}; use mz_compute_client::command::ReplicaId; use mz_compute_client::controller::ComputeInstanceId; @@ -31,14 +31,15 @@ use mz_repr::global_id::ProtoGlobalId; use mz_repr::GlobalId; use mz_sql::catalog::CatalogError as SqlCatalogError; use mz_sql::names::{ - DatabaseId, ObjectQualifiers, QualifiedObjectName, ResolvedDatabaseSpecifier, SchemaId, + DatabaseId, ObjectQualifiers, QualifiedObjectName, ResolvedDatabaseSpecifier, RoleId, SchemaId, SchemaSpecifier, }; use mz_sql::plan::ComputeInstanceIntrospectionConfig; use mz_stash::{Append, AppendBatch, Stash, StashError, TableTransaction, TypedCollection}; use mz_storage::types::sources::Timeline; -use crate::catalog::builtin::BuiltinLog; +use crate::catalog; +use crate::catalog::builtin::{BuiltinLog, BUILTIN_ROLES}; use crate::catalog::error::{Error, ErrorKind}; use crate::catalog::SerializedComputeInstanceReplicaConfig; use crate::catalog::SystemObjectMapping; @@ -57,7 +58,8 @@ const DEFAULT_REPLICA_ID: u64 = 1; const DATABASE_ID_ALLOC_KEY: &str = "database"; const SCHEMA_ID_ALLOC_KEY: &str = "schema"; -const ROLE_ID_ALLOC_KEY: &str = "role"; +const USER_ROLE_ID_ALLOC_KEY: &str = "user_role"; +const SYSTEM_ROLE_ID_ALLOC_KEY: &str = "system_role"; const COMPUTE_ID_ALLOC_KEY: &str = "compute"; const REPLICA_ID_ALLOC_KEY: &str = "replica"; pub(crate) const AUDIT_LOG_ID_ALLOC_KEY: &str = "auditlog"; @@ -112,12 +114,18 @@ async fn migrate( )?; txn.id_allocator.insert( IdAllocKey { - name: ROLE_ID_ALLOC_KEY.into(), + name: USER_ROLE_ID_ALLOC_KEY.into(), }, IdAllocValue { next_id: MATERIALIZE_ROLE_ID + 1, }, )?; + txn.id_allocator.insert( + IdAllocKey { + name: SYSTEM_ROLE_ID_ALLOC_KEY.into(), + }, + IdAllocValue { next_id: 1 }, + )?; txn.id_allocator.insert( IdAllocKey { name: COMPUTE_ID_ALLOC_KEY.into(), @@ -195,7 +203,7 @@ async fn migrate( )?; txn.roles.insert( RoleKey { - id: MATERIALIZE_ROLE_ID, + id: RoleId::User(MATERIALIZE_ROLE_ID).to_string(), }, RoleValue { name: "materialize".into(), @@ -496,12 +504,33 @@ impl Connection { .collect()) } - pub async fn load_roles(&mut self) -> Result, Error> { + pub async fn load_roles(&mut self) -> Result, Error> { + // Add in any new builtin roles. + let mut tx = self.transaction().await?; + let role_names: HashSet<_> = tx + .roles + .items() + .into_values() + .map(|value| value.name) + .collect(); + for builtin_role in &*BUILTIN_ROLES { + if !role_names.contains(builtin_role.name) { + tx.insert_system_role(builtin_role.name)?; + } + } + tx.commit().await?; + Ok(COLLECTION_ROLE .peek_one(&mut self.stash) .await? .into_iter() - .map(|(k, v)| (k.id, v.name)) + .map(|(k, v)| { + ( + RoleId::from_str(&k.id) + .unwrap_or_else(|_| panic!("Invalid persisted role id {}", k.id)), + v.name, + ) + }) .collect()) } @@ -945,10 +974,27 @@ impl<'a, S: Append> Transaction<'a, S> { } } - pub fn insert_role(&mut self, role_name: &str) -> Result { - let id = self.get_and_increment_id(ROLE_ID_ALLOC_KEY.to_string())?; + pub fn insert_user_role(&mut self, role_name: &str) -> Result { + self.insert_role(role_name, USER_ROLE_ID_ALLOC_KEY, RoleId::User) + } + + fn insert_system_role(&mut self, role_name: &str) -> Result { + self.insert_role(role_name, SYSTEM_ROLE_ID_ALLOC_KEY, RoleId::System) + } + + fn insert_role( + &mut self, + role_name: &str, + id_alloc_key: &str, + role_id_variant: F, + ) -> Result + where + F: Fn(u64) -> RoleId, + { + let id = self.get_and_increment_id(id_alloc_key.to_string())?; + let id = role_id_variant(id); match self.roles.insert( - RoleKey { id }, + RoleKey { id: id.to_string() }, RoleValue { name: role_name.to_string(), }, @@ -1654,8 +1700,8 @@ impl_codec!(ItemValue); #[derive(Clone, Message, PartialOrd, PartialEq, Eq, Ord, Hash)] struct RoleKey { - #[prost(uint64)] - id: u64, + #[prost(string)] + id: String, } impl_codec!(RoleKey); diff --git a/src/sql/src/catalog.rs b/src/sql/src/catalog.rs index 154a49aa2c6a..6f6ed74f3547 100644 --- a/src/sql/src/catalog.rs +++ b/src/sql/src/catalog.rs @@ -35,7 +35,7 @@ use uuid::Uuid; use crate::func::Func; use crate::names::{ Aug, DatabaseId, FullObjectName, PartialObjectName, QualifiedObjectName, QualifiedSchemaName, - ResolvedDatabaseSpecifier, SchemaSpecifier, + ResolvedDatabaseSpecifier, RoleId, SchemaSpecifier, }; use crate::plan::statement::StatementDesc; @@ -255,7 +255,7 @@ pub trait CatalogRole { fn name(&self) -> &str; /// Returns a stable ID for the role. - fn id(&self) -> u64; + fn id(&self) -> RoleId; } /// A compute instance in a [`SessionCatalog`]. diff --git a/src/sql/src/names.rs b/src/sql/src/names.rs index 2212c1fa6b62..caa0884c6c30 100644 --- a/src/sql/src/names.rs +++ b/src/sql/src/names.rs @@ -9,6 +9,7 @@ //! Structured name types for SQL objects. +use anyhow::anyhow; use std::collections::{HashMap, HashSet}; use std::fmt; use std::str::FromStr; @@ -663,6 +664,38 @@ impl FromStr for DatabaseId { } } +/// The identifier for a role. +#[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd, Hash, Serialize, Deserialize)] +pub enum RoleId { + System(u64), + User(u64), +} + +impl FromStr for RoleId { + type Err = anyhow::Error; + + fn from_str(s: &str) -> Result { + if s.len() < 2 { + return Err(anyhow!("couldn't parse role id {}", s)); + } + let val: u64 = s[1..].parse()?; + match s.chars().next().unwrap() { + 's' => Ok(Self::System(val)), + 'u' => Ok(Self::User(val)), + _ => Err(anyhow!("couldn't parse role id {}", s)), + } + } +} + +impl fmt::Display for RoleId { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + Self::System(id) => write!(f, "s{}", id), + Self::User(id) => write!(f, "u{}", id), + } + } +} + #[derive(Debug)] pub struct NameResolver<'a> { catalog: &'a dyn SessionCatalog, diff --git a/test/cluster/mzcompose.py b/test/cluster/mzcompose.py index 579b8568454b..3cfc1ae52cc7 100644 --- a/test/cluster/mzcompose.py +++ b/test/cluster/mzcompose.py @@ -8,7 +8,6 @@ # by the Apache License, Version 2.0. import time -from textwrap import dedent from pg8000.dbapi import ProgrammingError @@ -72,7 +71,6 @@ def workflow_default(c: Composition, parser: WorkflowArgumentParser) -> None: "test-github-12251", "test-remote-storaged", "test-drop-default-cluster", - "test-builtin-migration", "test-upsert", ]: with c.test_case(name): @@ -234,86 +232,3 @@ def workflow_test_drop_default_cluster(c: Composition) -> None: c.sql("DROP CLUSTER default CASCADE") c.sql("CREATE CLUSTER default REPLICAS (default (SIZE '1'))") - - -def workflow_test_builtin_migration(c: Composition) -> None: - """Exercise the builtin object migration code by upgrading between two versions - that will have a migration triggered between them. Create a materialized view - over the affected builtin object to confirm that the migration was successful - """ - - c.down(destroy_volumes=True) - with c.override( - Testdrive(default_timeout="15s", no_reset=True, consistent_seed=True), - ): - c.up("testdrive", persistent=True) - - # Use storage external to the mz image because the above sha used postgres but - # we now use cockroach, so the data are lost. - mz_options = " ".join( - [ - "--persist-consensus-url=postgresql://postgres:postgres@postgres:5432?options=--search_path=consensus", - "--storage-stash-url=postgresql://postgres:postgres@postgres:5432?options=--search_path=storage", - "--adapter-stash-url=postgresql://postgres:postgres@postgres:5432?options=--search_path=adapter", - ] - ) - - with c.override( - # This commit introduced the pg_authid builtin view with a missing column. The column was added in a - # later commit. - Materialized( - image="materialize/materialized:devel-4a26e59ac9da694d21b60c8d4d4a7b67c8b3b78d", - options=mz_options, - ), - Postgres(image="postgres:14.4"), - ): - c.up("postgres") - c.wait_for_postgres(service="postgres") - c.sql( - sql=f""" - CREATE SCHEMA IF NOT EXISTS consensus; - CREATE SCHEMA IF NOT EXISTS storage; - CREATE SCHEMA IF NOT EXISTS adapter; - """, - service="postgres", - user="postgres", - password="postgres", - ) - - c.up("materialized") - c.wait_for_materialized() - - c.testdrive( - input=dedent( - """ - > CREATE MATERIALIZED VIEW v1 AS SELECT COUNT(*) FROM pg_authid; - > CREATE DEFAULT INDEX ON v1; - > SELECT * FROM v1; - 2 - """ - ) - ) - - c.kill("materialized") - - with c.override( - # If this ever stops working, add the following argument: - # image="materialize/materialized:devel-438ea318093b3a15a924fbdae70e0db6d379a921" - # That commit added the missing column rolconnlimit to pg_authid. - Materialized(options=mz_options) - ): - c.up("materialized") - c.wait_for_materialized() - - c.testdrive( - input=dedent( - """ - > SELECT * FROM v1; - 2 - - # This column is new after the migration - > SELECT DISTINCT rolconnlimit FROM pg_authid; - -1 - """ - ) - ) diff --git a/test/sqllogictest/id_reuse.slt b/test/sqllogictest/id_reuse.slt index 116e71b19c6e..f2a0e95c489b 100644 --- a/test/sqllogictest/id_reuse.slt +++ b/test/sqllogictest/id_reuse.slt @@ -64,12 +64,12 @@ SELECT id, name FROM mz_databases statement ok CREATE ROLE foo LOGIN SUPERUSER -query IT rowsort +query TT rowsort SELECT id, name FROM mz_roles ---- -0 mz_system -1 materialize -2 foo +s1 mz_system +u1 materialize +u2 foo statement ok DROP ROLE foo @@ -77,12 +77,12 @@ DROP ROLE foo statement ok CREATE ROLE bar LOGIN SUPERUSER -query IT rowsort +query TT rowsort SELECT id, name FROM mz_roles ---- -0 mz_system -1 materialize -3 bar +s1 mz_system +u1 materialize +u3 bar statement ok CREATE CLUSTER foo REPLICAS (r1 (size '1')) diff --git a/test/sqllogictest/pg_catalog_roles.slt b/test/sqllogictest/pg_catalog_roles.slt index 961ad872992a..4c189eede178 100644 --- a/test/sqllogictest/pg_catalog_roles.slt +++ b/test/sqllogictest/pg_catalog_roles.slt @@ -12,5 +12,5 @@ mode cockroach query IT SELECT oid, rolname FROM pg_roles ORDER BY oid ---- -20007 materialize -20008 mz_system +20007 mz_system +20008 materialize diff --git a/test/testdrive/roles.td b/test/testdrive/roles.td index ae3880ebea39..c9069b47609a 100644 --- a/test/testdrive/roles.td +++ b/test/testdrive/roles.td @@ -11,8 +11,8 @@ $ set-sql-timeout duration=1s # Verify initial roles. > SELECT id, name FROM mz_roles -0 mz_system -1 materialize +s1 mz_system +u1 materialize # Verify that invalid options are rejected. ! CREATE ROLE foo @@ -28,37 +28,37 @@ contains:conflicting or redundant options > CREATE ROLE rj LOGIN SUPERUSER > CREATE USER fms SUPERUSER > SELECT id, name FROM mz_roles -0 mz_system -1 materialize -2 rj -3 fms +s1 mz_system +u1 materialize +u2 rj +u3 fms # Dropping multiple roles should not have any effect if one of the role names # is bad... ! DROP ROLE rj, fms, bad contains:unknown role 'bad' > SELECT id, name FROM mz_roles -0 mz_system -1 materialize -2 rj -3 fms +s1 mz_system +u1 materialize +u2 rj +u3 fms # ...unless IF EXISTS is specified. > DROP ROLE IF EXISTS rj, fms, bad > SELECT id, name FROM mz_roles -0 mz_system -1 materialize +s1 mz_system +u1 materialize # Verify that the single name version of DROP ROLE works too. > CREATE ROLE nlb LOGIN SUPERUSER > SELECT id, name FROM mz_roles -0 mz_system -1 materialize -4 nlb +s1 mz_system +u1 materialize +u4 nlb > DROP ROLE nlb > SELECT id, name FROM mz_roles -0 mz_system -1 materialize +s1 mz_system +u1 materialize > DROP ROLE IF EXISTS nlb # No dropping the current role.