From a5b1af1b3fa73d62005d61ff73268f45fc51a5a5 Mon Sep 17 00:00:00 2001
From: Georgy Moshkin <gmoshkin@picodata.io>
Date: Wed, 28 Aug 2024 19:08:55 +0300
Subject: [PATCH] refactor: move system parameter validation to config.rs
---
src/config.rs | 41 +++++++++++++++++++++++++++++++++++++++++
src/storage.rs | 47 +++++++----------------------------------------
2 files changed, 48 insertions(+), 40 deletions(-)
diff --git a/src/config.rs b/src/config.rs
index 999916676c..5675800038 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -26,8 +26,11 @@ use std::io::Write;
use std::path::Path;
use std::path::PathBuf;
use std::str::FromStr;
+use std::sync::atomic::{AtomicUsize, Ordering};
use tarantool::log::SayLevel;
use tarantool::tlua;
+use tarantool::tuple::RawBytes;
+use tarantool::tuple::Tuple;
/// This reexport is used in the derive macro for Introspection.
pub use sbroad::ir::relation::Type as SbroadType;
@@ -1522,6 +1525,44 @@ macro_rules! system_parameter_name {
}};
}
+/// Cached value of "pg_max_statements" option from "_pico_property".
+/// 0 means that the value must be read from the table.
+pub static MAX_PG_STATEMENTS: AtomicUsize = AtomicUsize::new(0);
+
+/// Cached value of "pg_max_portals" option from "_pico_property".
+/// 0 means that the value must be read from the table.
+pub static MAX_PG_PORTALS: AtomicUsize = AtomicUsize::new(0);
+
+pub fn validate_alter_system_parameter_tuple(name: &str, tuple: &Tuple) -> Result<(), Error> {
+ let Some(expected_type) = get_type_of_alter_system_parameter(name) else {
+ return Err(Error::other(format!("unknown parameter: '{name}'")));
+ };
+
+ let field_count = tuple.len();
+ if field_count != 2 {
+ #[rustfmt::skip]
+ return Err(Error::other(format!("too many fields: got {field_count}, expected 2")));
+ }
+
+ let raw_field = tuple.field::<&RawBytes>(1)?.expect("value is not nullable");
+ crate::util::check_msgpack_matches_type(raw_field, expected_type)?;
+
+ // TODO: implement caching for all `config::AlterSystemParameters`.
+ if name == system_parameter_name!(max_pg_portals) {
+ let value = tuple.field::<usize>(1)?.expect("type already checked");
+ // Cache the value.
+ MAX_PG_PORTALS.store(value, Ordering::Relaxed);
+ }
+
+ if name == system_parameter_name!(max_pg_statements) {
+ let value = tuple.field::<usize>(1)?.expect("type already checked");
+ // Cache the value.
+ MAX_PG_STATEMENTS.store(value, Ordering::Relaxed);
+ }
+
+ Ok(())
+}
+
/// 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 {
diff --git a/src/storage.rs b/src/storage.rs
index 8cc4069f3f..69d0e2729e 100644
--- a/src/storage.rs
+++ b/src/storage.rs
@@ -49,7 +49,6 @@ use crate::traft::RaftEntryId;
use crate::traft::RaftId;
use crate::traft::RaftIndex;
use crate::traft::Result;
-use crate::util::check_msgpack_matches_type;
use crate::util::Uppercase;
use crate::warn_or_panic;
@@ -63,7 +62,7 @@ use std::iter::Peekable;
use std::marker::PhantomData;
use std::rc::Rc;
use std::str::FromStr;
-use std::sync::atomic::{AtomicUsize, Ordering};
+use std::sync::atomic::Ordering;
use std::time::Duration;
use self::acl::{on_master_drop_role, on_master_drop_user};
@@ -1333,14 +1332,6 @@ impl PropertyName {
// Properties
////////////////////////////////////////////////////////////////////////////////
-/// Cached value of "pg_max_statements" option from "_pico_property".
-/// 0 means that the value must be read from the table.
-static MAX_PG_STATEMENTS: AtomicUsize = AtomicUsize::new(0);
-
-/// Cached value of "pg_max_portals" option from "_pico_property".
-/// 0 means that the value must be read from the table.
-static MAX_PG_PORTALS: AtomicUsize = AtomicUsize::new(0);
-
impl Properties {
pub fn new() -> tarantool::Result<Self> {
let space = Space::builder(Self::TABLE_NAME)
@@ -1409,34 +1400,10 @@ impl Properties {
.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"
- )));
- }
-
+ if config::get_type_of_alter_system_parameter(key).is_some() {
// 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`.
- if key == system_parameter_name!(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);
- }
- if key == system_parameter_name!(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);
- }
+ config::validate_alter_system_parameter_tuple(key, &new)?;
} else {
let Ok(key) = key.parse::<PropertyName>() else {
// Not a builtin property.
@@ -1563,7 +1530,7 @@ impl Properties {
#[inline]
pub fn max_pg_statements(&self) -> tarantool::Result<usize> {
- let cached = MAX_PG_STATEMENTS.load(Ordering::Relaxed);
+ let cached = config::MAX_PG_STATEMENTS.load(Ordering::Relaxed);
if cached != 0 {
return Ok(cached);
}
@@ -1571,13 +1538,13 @@ impl Properties {
let res = self.get_or_alter_system_default(system_parameter_name!(max_pg_statements))?;
// Cache the value.
- MAX_PG_STATEMENTS.store(res, Ordering::Relaxed);
+ config::MAX_PG_STATEMENTS.store(res, Ordering::Relaxed);
Ok(res)
}
#[inline]
pub fn max_pg_portals(&self) -> tarantool::Result<usize> {
- let cached = MAX_PG_PORTALS.load(Ordering::Relaxed);
+ let cached = config::MAX_PG_PORTALS.load(Ordering::Relaxed);
if cached != 0 {
return Ok(cached);
}
@@ -1585,7 +1552,7 @@ impl Properties {
let res = self.get_or_alter_system_default(system_parameter_name!(max_pg_portals))?;
// Cache the value.
- MAX_PG_PORTALS.store(res, Ordering::Relaxed);
+ config::MAX_PG_PORTALS.store(res, Ordering::Relaxed);
Ok(res)
}
--
GitLab