Skip to content

Commit

Permalink
feat(frontend): show if system parameters are mutable (#9526)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Gun9niR
Gun9niR authored Apr 28, 2023
1 parent 4fbdf3d commit 2e789bd
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 35 deletions.
74 changes: 42 additions & 32 deletions src/common/src/system_param/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,26 +26,26 @@ pub type SystemParamsError = String;
type Result<T> = core::result::Result<T, SystemParamsError>;

/// Only includes undeprecated params.
/// Macro input is { field identifier, type, default value }
/// Macro input is { field identifier, type, default value, is mutable }
///
/// Note:
/// - Having `None` as default value means the parameter must be initialized.
#[macro_export]
macro_rules! for_all_undeprecated_params {
($macro:ident
// Hack: match trailing fields to implement `for_all_params`
$(, { $field:ident, $type:ty, $default:expr })*) => {
$(, { $field:ident, $type:ty, $default:expr, $is_mutable:expr })*) => {
$macro! {
{ barrier_interval_ms, u32, Some(1000_u32) },
{ checkpoint_frequency, u64, Some(10_u64) },
{ sstable_size_mb, u32, Some(256_u32) },
{ block_size_kb, u32, Some(64_u32) },
{ bloom_false_positive, f64, Some(0.001_f64) },
{ state_store, String, None },
{ data_directory, String, None },
{ backup_storage_url, String, Some("memory".to_string()) },
{ backup_storage_directory, String, Some("backup".to_string()) },
{ telemetry_enabled, bool, Some(true) },
{ barrier_interval_ms, u32, Some(1000_u32), false },
{ checkpoint_frequency, u64, Some(10_u64), true },
{ sstable_size_mb, u32, Some(256_u32), false },
{ block_size_kb, u32, Some(64_u32), false },
{ bloom_false_positive, f64, Some(0.001_f64), false },
{ state_store, String, None, false },
{ data_directory, String, None, false },
{ backup_storage_url, String, Some("memory".to_string()), false },
{ backup_storage_directory, String, Some("backup".to_string()), false },
{ telemetry_enabled, bool, Some(true), true },
$({ $field, $type, $default },)*
}
};
Expand All @@ -72,7 +72,7 @@ macro_rules! key_of {

/// Define key constants for fields in `SystemParams` for use of other modules.
macro_rules! def_key {
($({ $field:ident, $type:ty, $default:expr },)*) => {
($({ $field:ident, $type:ty, $default:expr, $is_mutable:expr },)*) => {
paste! {
$(
pub const [<$field:upper _KEY>]: &str = key_of!($field);
Expand All @@ -85,7 +85,7 @@ for_all_params!(def_key);

/// Define default value functions.
macro_rules! def_default {
($({ $field:ident, $type:ty, $default:expr },)*) => {
($({ $field:ident, $type:ty, $default:expr, $is_mutable:expr },)*) => {
pub mod default {
$(
pub fn $field() -> Option<$type> {
Expand All @@ -100,7 +100,7 @@ for_all_undeprecated_params!(def_default);

/// Derive serialization to kv pairs.
macro_rules! impl_system_params_to_kv {
($({ $field:ident, $type:ty, $default:expr },)*) => {
($({ $field:ident, $type:ty, $default:expr, $is_mutable:expr },)*) => {
/// The returned map only contains undeprecated fields.
/// Return error if there are missing fields.
pub fn system_params_to_kv(params: &SystemParams) -> Result<Vec<(String, String)>> {
Expand All @@ -121,7 +121,7 @@ macro_rules! impl_system_params_to_kv {
}

macro_rules! impl_derive_missing_fields {
($({ $field:ident, $type:ty, $default:expr },)*) => {
($({ $field:ident, $type:ty, $default:expr, $is_mutable:expr },)*) => {
fn derive_missing_fields(params: &mut SystemParams) {
$(
if params.$field.is_none() && let Some(v) = OverrideFromParams::$field(params) {
Expand All @@ -134,7 +134,7 @@ macro_rules! impl_derive_missing_fields {

/// Derive deserialization from kv pairs.
macro_rules! impl_system_params_from_kv {
($({ $field:ident, $type:ty, $default:expr },)*) => {
($({ $field:ident, $type:ty, $default:expr, $is_mutable:expr },)*) => {
/// Try to deserialize deprecated fields as well.
/// Return error if there are unrecognized fields.
pub fn system_params_from_kv<K, V>(mut kvs: Vec<(K, V)>) -> Result<SystemParams>
Expand Down Expand Up @@ -168,23 +168,23 @@ macro_rules! impl_system_params_from_kv {
};
}

/// Define check rules when a field is changed. By default all fields are immutable.
/// Define check rules when a field is changed.
/// If you want custom rules, please override the default implementation in
/// `OverrideValidateOnSet` below.
macro_rules! impl_default_validation_on_set {
($({ $field:ident, $type:ty, $default:expr },)*) => {
($({ $field:ident, $type:ty, $default:expr, $is_mutable:expr },)*) => {
#[allow(clippy::ptr_arg)]
trait ValidateOnSet {
$(
fn $field(_v: &$type) -> Result<()> {
Self::expect_immutable(key_of!($field))
if !$is_mutable {
Err(format!("{:?} is immutable", key_of!($field)))
} else {
Ok(())
}
}
)*

fn expect_immutable(field: &str) -> Result<()> {
Err(format!("{:?} is immutable", field))
}

fn expect_range<T, R>(v: T, range: R) -> Result<()>
where
T: Debug + PartialOrd,
Expand Down Expand Up @@ -221,7 +221,7 @@ macro_rules! impl_default_validation_on_set {
///
/// Note that newer versions must be prioritized during derivation.
macro_rules! impl_default_from_other_params {
($({ $field:ident, $type:ty, $default:expr },)*) => {
($({ $field:ident, $type:ty, $default:expr, $is_mutable:expr },)*) => {
trait FromParams {
$(
fn $field(_params: &SystemParams) -> Option<$type> {
Expand All @@ -233,7 +233,7 @@ macro_rules! impl_default_from_other_params {
}

macro_rules! impl_set_system_param {
($({ $field:ident, $type:ty, $default:expr },)*) => {
($({ $field:ident, $type:ty, $default:expr, $is_mutable:expr },)*) => {
pub fn set_system_param(params: &mut SystemParams, key: &str, value: Option<String>) -> Result<()> {
match key {
$(
Expand All @@ -259,8 +259,21 @@ macro_rules! impl_set_system_param {
};
}

macro_rules! impl_is_mutable {
($({ $field:ident, $type:ty, $default:expr, $is_mutable:expr },)*) => {
pub fn is_mutable(field: &str) -> Result<bool> {
match field {
$(
key_of!($field) => Ok($is_mutable),
)*
_ => Err(format!("{:?} is not a system parameter", field))
}
}
}
}

macro_rules! impl_system_params_for_test {
($({ $field:ident, $type:ty, $default:expr },)*) => {
($({ $field:ident, $type:ty, $default:expr, $is_mutable:expr },)*) => {
#[allow(clippy::needless_update)]
pub fn system_params_for_test() -> SystemParams {
let mut ret = SystemParams {
Expand All @@ -276,8 +289,9 @@ macro_rules! impl_system_params_for_test {
};
}

for_all_undeprecated_params!(impl_derive_missing_fields);
for_all_params!(impl_system_params_from_kv);
for_all_params!(impl_is_mutable);
for_all_undeprecated_params!(impl_derive_missing_fields);
for_all_undeprecated_params!(impl_system_params_to_kv);
for_all_undeprecated_params!(impl_set_system_param);
for_all_undeprecated_params!(impl_default_validation_on_set);
Expand All @@ -298,10 +312,6 @@ impl ValidateOnSet for OverrideValidateOnSet {
// TODO
Ok(())
}

fn telemetry_enabled(_: &bool) -> Result<()> {
Ok(())
}
}

for_all_undeprecated_params!(impl_default_from_other_params);
Expand Down
15 changes: 13 additions & 2 deletions src/frontend/src/handler/variable.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ use pgwire::pg_field_descriptor::PgFieldDescriptor;
use pgwire::pg_response::{PgResponse, StatementType};
use pgwire::types::Row;
use risingwave_common::error::{ErrorCode, Result};
use risingwave_common::types::DataType;
use risingwave_common::system_param::is_mutable;
use risingwave_common::types::{DataType, ScalarRefImpl};
use risingwave_sqlparser::ast::{Ident, SetTimeZoneValue, SetVariableValue, Value};

use super::RwPgResponse;
Expand Down Expand Up @@ -145,7 +146,12 @@ async fn handle_show_system_params(handler_args: HandlerArgs) -> Result<RwPgResp
let rows = params
.to_kv()
.into_iter()
.map(|(k, v)| Row::new(vec![Some(k.into()), Some(v.into())]))
.map(|(k, v)| {
let is_mutable_bytes = ScalarRefImpl::Bool(is_mutable(&k).unwrap())
.text_format(&DataType::Boolean)
.into();
Row::new(vec![Some(k.into()), Some(v.into()), Some(is_mutable_bytes)])
})
.collect_vec();

Ok(RwPgResponse::new_for_stream(
Expand All @@ -163,6 +169,11 @@ async fn handle_show_system_params(handler_args: HandlerArgs) -> Result<RwPgResp
DataType::Varchar.to_oid(),
DataType::Varchar.type_len(),
),
PgFieldDescriptor::new(
"Mutable".to_string(),
DataType::Boolean.to_oid(),
DataType::Boolean.type_len(),
),
],
))
}
2 changes: 1 addition & 1 deletion src/meta/src/manager/system_param/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ impl<S: MetaStore> SystemParamsManager<S> {
// params are not set. Use init value.
// 4. None, None: Impossible.
macro_rules! impl_merge_params {
($({ $field:ident, $type:ty, $default:expr },)*) => {
($({ $field:ident, $type:ty, $default:expr, $is_mutable:expr },)*) => {
fn merge_params(mut persisted: SystemParams, init: SystemParams) -> SystemParams {
$(
match (persisted.$field.as_ref(), init.$field) {
Expand Down

0 comments on commit 2e789bd

Please sign in to comment.