From 9463a82bfb89e1411febdb7c2cede54f2305e391 Mon Sep 17 00:00:00 2001 From: Georgy Moshkin <gmoshkin@picodata.io> Date: Wed, 28 Aug 2024 17:37:44 +0300 Subject: [PATCH] feat: support the rest of parameters in alter system + big refactor Add new config::AlterSystemParameters struct which is responsible for definitions of system parameters, their default values and mapping to sbroad type. This is a big step towards moving all system parameters from _pico_property into another system table. --- src/config.rs | 155 ++++++++++++++++++++++++++++++++++++++++ src/sql.rs | 85 +++++++++++----------- src/storage.rs | 125 +++++++++++++++----------------- src/util.rs | 165 +++++++++++++++++++++++++++++++++++++++++++ test/int/test_sql.py | 14 +++- 5 files changed, 432 insertions(+), 112 deletions(-) diff --git a/src/config.rs b/src/config.rs index 69acf5dac7..ea5796c888 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1390,6 +1390,161 @@ tarantool::define_str_enum! { } } +//////////////////////////////////////////////////////////////////////////////// +// AlterSystemParameters +//////////////////////////////////////////////////////////////////////////////// + +/// A struct which represents an enumeration of system parameters which can be +/// configured via ALTER SYSTEM. +/// +/// This struct uses the `Introspection` trait to associate with each paramater +/// a `SbroadType` and a default value. +/// +/// At the moment this struct is never actually used as a struct, but only as +/// a container for the metadata described above. +#[derive(PartialEq, Default, Debug, Clone, serde::Deserialize, serde::Serialize, Introspection)] +#[serde(deny_unknown_fields)] +pub struct AlterSystemParameters { + /// Password should contain at least this many characters + #[introspection(sbroad_type = SbroadType::Unsigned)] + #[introspection(config_default = 8)] + pub password_min_length: u64, + + /// Password should contain at least one uppercase letter + #[introspection(sbroad_type = SbroadType::Boolean)] + #[introspection(config_default = true)] + pub password_enforce_uppercase: bool, + + /// Password should contain at least one lowercase letter + #[introspection(sbroad_type = SbroadType::Boolean)] + #[introspection(config_default = true)] + pub password_enforce_lowercase: bool, + + /// Password should contain at least one digit + #[introspection(sbroad_type = SbroadType::Boolean)] + #[introspection(config_default = true)] + pub password_enforce_digits: bool, + + /// Password should contain at least one special symbol. + /// Special symbols - &, |, ?, !, $, @ + #[introspection(sbroad_type = SbroadType::Boolean)] + #[introspection(config_default = false)] + pub password_enforce_specialchars: bool, + + /// Maximum number of login attempts through `picodata connect`. + /// Each failed login attempt increases a local per user counter of failed attempts. + /// When the counter reaches the value of this property any subsequent logins + /// of this user will be denied. + /// Local counter for a user is reset on successful login. + #[introspection(sbroad_type = SbroadType::Unsigned)] + #[introspection(config_default = 4)] + pub max_login_attempts: u64, + + /// Number of seconds to wait before automatically changing an + /// unresponsive instance's state to Offline. + #[introspection(sbroad_type = SbroadType::Double)] + #[introspection(config_default = 5.0)] + pub auto_offline_timeout: f64, + + /// Maximum number of seconds to wait before sending another heartbeat + /// to an unresponsive instance. + #[introspection(sbroad_type = SbroadType::Double)] + #[introspection(config_default = 5.0)] + pub max_heartbeat_period: f64, + + /// PG statement storage size. + #[introspection(sbroad_type = SbroadType::Unsigned)] + #[introspection(config_default = 1024)] + pub max_pg_statements: u64, + + /// PG portal storage size. + #[introspection(sbroad_type = SbroadType::Unsigned)] + #[introspection(config_default = 1024)] + pub max_pg_portals: u64, + + /// Raft snapshot will be sent out in chunks not bigger than this threshold. + /// Note: actual snapshot size may exceed this threshold. In most cases + /// it will just add a couple of dozen metadata bytes. But in extreme + /// cases if there's a tuple larger than this threshold, it will be sent + /// in one piece whatever size it has. Please don't store tuples of size + /// greater than this. + #[introspection(sbroad_type = SbroadType::Unsigned)] + #[introspection(config_default = 16 * 1024 * 1024)] + pub snapshot_chunk_max_size: u64, + + /// Snapshot read views with live reference counts will be forcefully + /// closed after this number of seconds. This is necessary if followers + /// do not properly finalize the snapshot application. + // NOTE: maybe we should instead track the instance/raft ids of + // followers which requested the snapshots and automatically close the + // read views if the corresponding instances are *deteremined* to not + // need them anymore. Or maybe timeouts is the better way.. + #[introspection(sbroad_type = SbroadType::Double)] + #[introspection(config_default = (24 * 3600))] + pub snapshot_read_view_close_timeout: f64, +} + +/// Returns `None` if there's no such parameter. +pub fn get_type_of_alter_system_parameter(name: &str) -> Option<SbroadType> { + let Ok(typ) = AlterSystemParameters::get_sbroad_type_of_field(name) else { + return None; + }; + + let typ = typ.expect("types must be specified for all parameters"); + Some(typ) +} + +/// Returns `None` if there's no such parameter. +pub fn get_default_value_of_alter_system_parameter(name: &str) -> Option<rmpv::Value> { + // NOTE: we need an instance of this struct because of how `Introspection::get_field_default_value_as_rmpv` + // works. We allow default values to refer to other fields of the struct + // which were actuall provided (see `InstanceConfig::advertise_address` for example). + // But for alter system parameters it doesn't make sense because these are not + // stored in the struct itself but in a system table. + // Alter system parameters currently don't delegate their defaults to other + // fields, so we just construct a default value of the struct ad hoc when + // needed. + // + // In the future maybe we want to maintain the values of the system + // parameters in an actual struct and update those every time the values in + // the system table change. + let parameters = AlterSystemParameters::default(); + + let Ok(default) = parameters.get_field_default_value_as_rmpv(name) else { + return None; + }; + + let default = default.expect("default must be specified explicitly for all parameters"); + Some(default) +} + +/// Returns an array of pairs (parameter name, default value). +pub fn get_defaults_for_all_alter_system_parameters() -> Vec<(String, rmpv::Value)> { + // NOTE: we need an instance of this struct because of how `Introspection::get_field_default_value_as_rmpv` + // works. We allow default values to refer to other fields of the struct + // which were actuall provided (see `InstanceConfig::advertise_address` for example). + // But for alter system parameters it doesn't make sense because these are not + // stored in the struct itself but in a system table. + // Alter system parameters currently don't delegate their defaults to other + // fields, so we just construct a default value of the struct ad hoc when + // needed. + // + // In the future maybe we want to maintain the values of the system + // parameters in an actual struct and update those every time the values in + // the system table change. + let parameters = AlterSystemParameters::default(); + + let mut result = Vec::with_capacity(AlterSystemParameters::FIELD_INFOS.len()); + for name in &leaf_field_paths::<AlterSystemParameters>() { + let default = parameters + .get_field_default_value_as_rmpv(name) + .expect("paths are correct"); + let default = default.expect("default must be specified explicitly for all parameters"); + result.push((name.clone(), default)); + } + result +} + //////////////////////////////////////////////////////////////////////////////// // deserialize_map_forbid_duplicate_keys //////////////////////////////////////////////////////////////////////////////// diff --git a/src/sql.rs b/src/sql.rs index 7db375f36b..788875cce6 100644 --- a/src/sql.rs +++ b/src/sql.rs @@ -11,7 +11,7 @@ use crate::schema::{ }; use crate::sql::router::RouterRuntime; use crate::sql::storage::StorageRuntime; -use crate::storage::{space_by_name, PropertyName}; +use crate::storage::space_by_name; use crate::sync::wait_for_index_globally; use crate::traft::error::{self, Error, Unsupported}; use crate::traft::node::Node as TraftNode; @@ -1121,58 +1121,47 @@ fn alter_system_ir_node_to_op_or_result( ))); } - let parse_property_name = |param_name: &str| -> traft::Result<PropertyName> { - param_name - .parse::<PropertyName>() - .map_err(|_| Error::other(format!("unknown property: '{param_name}'"))) - }; - + let table = crate::storage::ClusterwideTable::Property; + let initiator = current_user; match ty { AlterSystemType::AlterSystemSet { param_name, param_value, } => { - let key = parse_property_name(param_name)?; - let tuple = crate::storage::property_key_value_to_tuple(key, param_value)?; + let Some(expected_type) = crate::config::get_type_of_alter_system_parameter(param_name) + else { + return Err(Error::other(format!("unknown parameter: '{param_name}'"))); + }; + let Ok(casted_value) = param_value.cast(&expected_type) else { + let actual_type = value_type_str(param_value); + return Err(Error::other(format!( + "invalid value for '{param_name}' expected {expected_type}, got {actual_type}", + ))); + }; + let dml = Dml::replace(table, &(param_name, casted_value), initiator)?; - Ok(Continue(Op::Dml(Dml::Replace { - table: crate::storage::ClusterwideTable::Property.into(), - tuple, - initiator: current_user, - }))) + Ok(Continue(Op::Dml(dml))) } AlterSystemType::AlterSystemReset { param_name } => { match param_name { - Some(key) => { + Some(param_name) => { // reset one - let key = parse_property_name(key)?; - let tuple = crate::storage::default_property_tuple(key)?; - - Ok(Continue(Op::Dml(Dml::Replace { - table: crate::storage::ClusterwideTable::Property.into(), - tuple, - initiator: current_user, - }))) + let Some(default_value) = + crate::config::get_default_value_of_alter_system_parameter(param_name) + else { + return Err(Error::other(format!("unknown parameter: '{param_name}'"))); + }; + let dml = Dml::replace(table, &(param_name, default_value), initiator)?; + + Ok(Continue(Op::Dml(dml))) } None => { // reset all - use PropertyName::*; - - let mut dmls = vec![]; - for prop in [ - PasswordMinLength, - PasswordEnforceDigits, - AutoOfflineTimeout, - MaxHeartbeatPeriod, - SnapshotChunkMaxSize, - SnapshotReadViewCloseTimeout, - ] { - let tuple = crate::storage::default_property_tuple(prop)?; - dmls.push(Dml::Replace { - table: crate::storage::ClusterwideTable::Property.into(), - tuple, - initiator: current_user, - }) + let defaults = crate::config::get_defaults_for_all_alter_system_parameters(); + let mut dmls = Vec::with_capacity(defaults.len()); + for (param_name, default_value) in defaults { + let dml = Dml::replace(table, &(param_name, default_value), initiator)?; + dmls.push(dml); } Ok(Continue(Op::BatchDml { ops: dmls })) } @@ -1785,3 +1774,19 @@ fn do_dml_on_global_tbl(mut query: Query<RouterRuntime>) -> traft::Result<Consum }) })? } + +// TODO: move this to sbroad +fn value_type_str(value: &Value) -> &'static str { + match value { + Value::Boolean { .. } => "boolean", + Value::Decimal { .. } => "decimal", + Value::Double { .. } => "double", + Value::Datetime { .. } => "datetime", + Value::Integer { .. } => "integer", + Value::Null { .. } => "null", + Value::String { .. } => "string", + Value::Unsigned { .. } => "unsigned", + Value::Tuple { .. } => "tuple", + Value::Uuid { .. } => "uuid", + } +} diff --git a/src/storage.rs b/src/storage.rs index 50758b4da8..fe92a48c13 100644 --- a/src/storage.rs +++ b/src/storage.rs @@ -65,8 +65,6 @@ use std::time::Duration; use self::acl::{on_master_drop_role, on_master_drop_user}; use pico_proc_macro::get_doc_literal; -use sbroad::ir::relation::Type; -use sbroad::ir::value::Value; macro_rules! define_clusterwide_tables { ( @@ -1407,40 +1405,6 @@ impl PropertyName { // Properties //////////////////////////////////////////////////////////////////////////////// -pub(crate) fn property_key_value_to_tuple(key: PropertyName, value: &Value) -> Result<TupleBuffer> { - use PropertyName::*; - - let property_expected_type = match key { - PasswordEnforceDigits => Type::Boolean, - PasswordMinLength | AutoOfflineTimeout | SnapshotChunkMaxSize => Type::Unsigned, - MaxHeartbeatPeriod | SnapshotReadViewCloseTimeout => Type::Double, - key => return Err(Error::other(format!("unknown property: '{key}'"))), - }; - let casted_value = value.cast(&property_expected_type).map_err(|_| { - Error::other(format!( - "'{key}' property expected value of {property_expected_type} type." - )) - })?; - (key, casted_value).to_tuple_buffer().map_err(Error::other) -} - -pub(crate) fn default_property_tuple(key: PropertyName) -> Result<TupleBuffer> { - use PropertyName::*; - let tuple = match key { - PasswordMinLength => (key, DEFAULT_PASSWORD_MIN_LENGTH).to_tuple_buffer(), - PasswordEnforceDigits => (key, DEFAULT_PASSWORD_ENFORCE_DIGITS).to_tuple_buffer(), - AutoOfflineTimeout => (key, DEFAULT_AUTO_OFFLINE_TIMEOUT).to_tuple_buffer(), - MaxHeartbeatPeriod => (key, DEFAULT_MAX_HEARTBEAT_PERIOD).to_tuple_buffer(), - SnapshotChunkMaxSize => (key, DEFAULT_SNAPSHOT_CHUNK_MAX_SIZE).to_tuple_buffer(), - SnapshotReadViewCloseTimeout => { - (key, DEFAULT_SNAPSHOT_READ_VIEW_CLOSE_TIMEOUT).to_tuple_buffer() - } - key => return Err(Error::other(format!("unknown property: '{key}'"))), - }; - - tuple.map_err(Error::other) -} - pub const DEFAULT_PASSWORD_MIN_LENGTH: usize = 8; pub const DEFAULT_PASSWORD_ENFORCE_UPPERCASE: bool = true; pub const DEFAULT_PASSWORD_ENFORCE_LOWERCASE: bool = true; @@ -1524,43 +1488,66 @@ impl Properties { } (old, Some(new)) => { // Insert or Update - let Ok(Some(key)) = new.field::<PropertyName>(0) else { - // Not a builtin property. - // Cannot be a wrong type error, because tarantool checks - // the format for us. - if old.is_none() { - // Insert - // FIXME: this is currently printed twice - tlog!(Warning, "non builtin property inserted into _pico_property, this may be an error in a future version of picodata"); - } - return Ok(()); - }; - - key.verify_new_tuple(&new)?; - let field_count = new.len(); - if field_count != 2 { - return Err(Error::other(format!( - "too many fields: got {field_count}, expected 2" - ))); - } + let key = new + .field::<&str>(0) + .expect("key has type string") + .expect("key is not nullable"); + + if let Some(expected_type) = config::get_type_of_alter_system_parameter(key) { + let field_count = new.len(); + if field_count != 2 { + return Err(Error::other(format!( + "too many fields: got {field_count}, expected 2" + ))); + } - match key { - PropertyName::MaxPgPortals => { - let value = new - .field::<usize>(1)? - .expect("just verified with verify_new_tuple"); - // Cache the value. - MAX_PG_PORTALS.store(value, Ordering::Relaxed); + // This is an alter system parameter. + // TODO: move these to a separate table + let raw_field = new.field::<&RawBytes>(1)?.expect("value is not nullable"); + check_msgpack_matches_type(raw_field, expected_type)?; + + // TODO: implement caching for all `config::AlterSystemParameters`. + match key { + // TODO: type safety + "max_pg_portals" => { + let value = new + .field::<usize>(1)? + .expect("just verified with verify_new_tuple"); + // Cache the value. + MAX_PG_PORTALS.store(value, Ordering::Relaxed); + } + // TODO: type safety + "max_pg_statements" => { + let value = new + .field::<usize>(1)? + .expect("just verified with verify_new_tuple"); + // Cache the value. + MAX_PG_STATEMENTS.store(value, Ordering::Relaxed); + } + _ => (), } - PropertyName::MaxPgStatements => { - let value = new - .field::<usize>(1)? - .expect("just verified with verify_new_tuple"); - // Cache the value. - MAX_PG_STATEMENTS.store(value, Ordering::Relaxed); + } else { + let Ok(key) = key.parse::<PropertyName>() else { + // Not a builtin property. + // Cannot be a wrong type error, because tarantool checks + // the format for us. + if old.is_none() { + // Insert + // FIXME: this is currently printed twice + tlog!(Warning, "non builtin property inserted into _pico_property, this may be an error in a future version of picodata"); + } + return Ok(()); + }; + + let field_count = new.len(); + if field_count != 2 { + return Err(Error::other(format!( + "too many fields: got {field_count}, expected 2" + ))); } - _ => (), + + key.verify_new_tuple(&new)?; } } (None, None) => unreachable!(), diff --git a/src/util.rs b/src/util.rs index a46be3600a..49fb5452a7 100644 --- a/src/util.rs +++ b/src/util.rs @@ -1,3 +1,4 @@ +use crate::config::SbroadType; use crate::traft::error::Error; use nix::sys::termios::{tcgetattr, tcsetattr, LocalFlags, SetArg::TCSADRAIN}; use std::any::{Any, TypeId}; @@ -825,6 +826,170 @@ pub fn check_tuple_matches_format(tuple: &[u8], format: &[Field], what_to_fix: & } } +// TODO: this should be in sbroad +pub fn check_msgpack_matches_type( + msgpack: &[u8], + expected_type: SbroadType, +) -> crate::traft::Result<()> { + use rmp::Marker; + + let mut cursor = msgpack; + let marker = rmp::decode::read_marker(&mut cursor).map_err(|e| + // XXX: here format the error using Debug because rmp doesn't implement Display for the error type. + // This is very bad. You should always implement Display for error types and format them using Display, not Debug! + Error::other(format!("{e:?}")))?; + let ok = match expected_type { + SbroadType::Any => true, + SbroadType::Map => { + matches!( + marker, + Marker::FixMap { .. } | Marker::Map16 | Marker::Map32 + ) + } + SbroadType::Array => { + matches!( + marker, + Marker::FixArray { .. } | Marker::Array16 | Marker::Array32 + ) + } + SbroadType::Boolean => { + matches!(marker, Marker::True | Marker::False) + } + SbroadType::Datetime => { + // FIXME: should check actual extension type + is_ext(marker) + } + SbroadType::Decimal | SbroadType::Number => { + // FIXME: should check actual extension type + // XXX: Also is this even correct? + is_ext(marker) || is_int(marker) || is_float(marker) + } + SbroadType::Double => is_int(marker) || is_float(marker), + SbroadType::Integer => is_int(marker), + SbroadType::Scalar => { + is_ext(marker) || is_int(marker) || is_float(marker) || is_str(marker) || is_bin(marker) + } + SbroadType::String => { + is_str(marker) || + // XXX: is this correct? + is_bin(marker) + } + SbroadType::Uuid => { + // FIXME: should check actual extension type + is_ext(marker) + } + SbroadType::Unsigned => is_uint(marker), + }; + + if !ok { + return Err(Error::other(format!( + "invalid type: expected {expected_type}, got {}", + mp_type_name(marker), + ))); + } + + return Ok(()); + + #[inline(always)] + fn is_ext(marker: Marker) -> bool { + matches!( + marker, + Marker::FixExt1 + | Marker::FixExt2 + | Marker::FixExt4 + | Marker::FixExt8 + | Marker::FixExt16 + | Marker::Ext8 + | Marker::Ext16 + | Marker::Ext32 + ) + } + + #[inline(always)] + fn is_int(marker: Marker) -> bool { + matches!( + marker, + Marker::FixPos { .. } + | Marker::FixNeg { .. } + | Marker::U8 + | Marker::U16 + | Marker::U32 + | Marker::U64 + | Marker::I8 + | Marker::I16 + | Marker::I32 + | Marker::I64 + ) + } + + #[inline(always)] + fn is_uint(marker: Marker) -> bool { + matches!( + marker, + Marker::FixPos { .. } | Marker::U8 | Marker::U16 | Marker::U32 | Marker::U64 + ) + } + + #[inline(always)] + fn is_float(marker: Marker) -> bool { + matches!(marker, Marker::F32 | Marker::F64) + } + + #[inline(always)] + fn is_str(marker: Marker) -> bool { + matches!( + marker, + Marker::FixStr { .. } | Marker::Str8 | Marker::Str16 | Marker::Str32 + ) + } + + #[inline(always)] + fn is_bin(marker: Marker) -> bool { + matches!(marker, Marker::Bin8 | Marker::Bin16 | Marker::Bin32) + } + + #[rustfmt::skip] + fn mp_type_name(marker: Marker) -> &'static str { + match marker { + Marker::Null => "null", + Marker::True | Marker::False => "boolean", + Marker::FixPos { .. } + | Marker::U8 + | Marker::U16 + | Marker::U32 + | Marker::U64 => "unsigned", + Marker::FixNeg { .. } + | Marker::I8 + | Marker::I16 + | Marker::I32 + | Marker::I64 => "integer", + Marker::F32 | Marker::F64 => "double", + Marker::FixStr { .. } + | Marker::Str8 + | Marker::Str16 + | Marker::Str32 => "string", + Marker::Bin8 + | Marker::Bin16 + | Marker::Bin32 => "binary", + Marker::FixArray { .. } + | Marker::Array16 + | Marker::Array32 => "array", + Marker::FixMap { .. } + | Marker::Map16 + | Marker::Map32 => "map", + Marker::FixExt1 + | Marker::FixExt2 + | Marker::FixExt4 + | Marker::FixExt8 + | Marker::FixExt16 + | Marker::Ext8 + | Marker::Ext16 + | Marker::Ext32 => "msgpack extension", + Marker::Reserved => "<reserved>", + } + } +} + /// Returns the number of character edit operations needed to convert `lhs` to `rhs`. /// /// By operations we mean diff --git a/test/int/test_sql.py b/test/int/test_sql.py index 527b3810b0..0e293af01a 100644 --- a/test/int/test_sql.py +++ b/test/int/test_sql.py @@ -5058,6 +5058,12 @@ def test_alter_system_property(cluster: Cluster): ("max_heartbeat_period", 6.6), ("snapshot_chunk_max_size", 1500), ("snapshot_read_view_close_timeout", 12312.4), + ("password_enforce_uppercase", False), + ("password_enforce_lowercase", False), + ("password_enforce_specialchars", True), + ("max_login_attempts", 8), + ("max_pg_statements", 4096), + ("max_pg_portals", 2048), ] default_prop = [] @@ -5141,7 +5147,7 @@ def test_alter_system_property_errors(cluster: Cluster): # such property does not exist with pytest.raises( - TarantoolError, match="unknown property: 'invalid_parameter_name'" + TarantoolError, match="unknown parameter: 'invalid_parameter_name'" ): dml = i1.sql( """ @@ -5152,7 +5158,7 @@ def test_alter_system_property_errors(cluster: Cluster): # property expects different value type with pytest.raises( TarantoolError, - match="'password_enforce_digits' property expected value of boolean type.", + match="invalid value for 'password_enforce_digits' expected boolean, got unsigned.", ): dml = i1.sql( """ @@ -5161,7 +5167,9 @@ def test_alter_system_property_errors(cluster: Cluster): ) # such property exists but must not be allowed to be changed through alter system - with pytest.raises(TarantoolError, match="unknown property: 'next_schema_version'"): + with pytest.raises( + TarantoolError, match="unknown parameter: 'next_schema_version'" + ): dml = i1.sql( """ alter system set "next_schema_version" to 3 -- GitLab