diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7e447a5b6189e9b031df627c9f67a048a22d481b..ae8c3650707910a5d98245298d4cfd4af9bfa1ed 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -36,6 +36,8 @@ with the `YY.MINOR.MICRO` scheme.
- Clusterwide SQL supports procedure calling.
+- Clusterwide SQL supports procedure renaming.
+
-->
## [24.1.1] - 2024-02-09
diff --git a/sbroad b/sbroad
index 92b200a41109d4c5137eba30c64bb8d10a090465..1dd868bef2b31189d47e7ac74e5ba29ff6912a02 160000
--- a/sbroad
+++ b/sbroad
@@ -1 +1 @@
-Subproject commit 92b200a41109d4c5137eba30c64bb8d10a090465
+Subproject commit 1dd868bef2b31189d47e7ac74e5ba29ff6912a02
diff --git a/src/access_control.rs b/src/access_control.rs
index 5040ac75ed36c47187861c61b50e84865761ac06..40b2bde0b2ec576219146a86a63b935771643344 100644
--- a/src/access_control.rs
+++ b/src/access_control.rs
@@ -242,6 +242,19 @@ fn access_check_ddl(ddl: &op::Ddl, as_user: UserId) -> tarantool::Result<()> {
as_user,
)
}
+ op::Ddl::RenameProcedure {
+ routine_id,
+ old_name,
+ owner_id,
+ ..
+ } => box_access_check_ddl_as_user(
+ old_name,
+ *routine_id,
+ *owner_id,
+ TntSchemaObjectType::Function,
+ PrivType::Alter,
+ as_user,
+ ),
}
}
diff --git a/src/cas.rs b/src/cas.rs
index c689ce27596a770a3515344f8bb903164f2aebde..87cedb06e817cf081278d29b0328c33aabf7e6d8 100644
--- a/src/cas.rs
+++ b/src/cas.rs
@@ -735,6 +735,7 @@ fn modifies_operable(op: &Op, space: SpaceId, storage: &Clusterwide) -> bool {
Ddl::DropIndex { .. } => false,
Ddl::CreateProcedure { .. } => false,
Ddl::DropProcedure { .. } => false,
+ Ddl::RenameProcedure { .. } => false,
};
match op {
Op::DdlPrepare { ddl, .. } => ddl_modifies(ddl),
diff --git a/src/rpc/ddl_apply.rs b/src/rpc/ddl_apply.rs
index dc4ed5ff940894f13c8240c379cf1db94abf61e9..f3fc639e6a8d0ef05d87a24231ebb364335f5b3e 100644
--- a/src/rpc/ddl_apply.rs
+++ b/src/rpc/ddl_apply.rs
@@ -1,9 +1,9 @@
use crate::op::Ddl;
-use crate::storage::Clusterwide;
use crate::storage::{
ddl_create_function_on_master, ddl_create_space_on_master, ddl_drop_function_on_master,
ddl_drop_space_on_master,
};
+use crate::storage::{ddl_rename_function_on_master, Clusterwide};
use crate::storage::{local_schema_version, set_local_schema_version};
use crate::tlog;
use crate::traft::error::Error as TraftError;
@@ -155,6 +155,17 @@ pub fn apply_schema_change(
}
}
+ Ddl::RenameProcedure {
+ routine_id,
+ ref old_name,
+ ref new_name,
+ ..
+ } => {
+ if let Err(e) = ddl_rename_function_on_master(routine_id, old_name, new_name) {
+ return Err(Error::Aborted(e.to_string()));
+ }
+ }
+
_ => {
todo!();
}
diff --git a/src/schema.rs b/src/schema.rs
index 3dca7918f4a29f1b35115e42dd00668a619a55c6..30a6c61965dca49f29ee24b97d1859dbdd12bff6 100644
--- a/src/schema.rs
+++ b/src/schema.rs
@@ -1,4 +1,4 @@
-use sbroad::ir::ddl::Language;
+use sbroad::ir::ddl::{Language, ParamDef};
use std::borrow::Cow;
use std::collections::{BTreeMap, HashSet};
use std::fmt::Display;
@@ -1031,6 +1031,8 @@ impl Encode for RoutineDef {}
impl RoutineDef {
/// Index (0-based) of field "operable" in _pico_routine table format.
pub const FIELD_OPERABLE: usize = 8;
+ /// Index (0-based) of field "name" in _pico_routine table format.
+ pub const FIELD_NAME: usize = 1;
/// Format of the _pico_routine global table.
#[inline(always)]
@@ -1198,6 +1200,23 @@ impl From<Field> for tarantool::space::Field {
}
}
+#[derive(Clone, Debug)]
+pub struct RenameRoutineParams {
+ pub new_name: String,
+ pub old_name: String,
+ pub params: Option<Vec<ParamDef>>,
+}
+
+impl RenameRoutineParams {
+ pub fn func_exists(&self) -> bool {
+ func_exists(&self.old_name)
+ }
+
+ pub fn new_name_occupied(&self) -> bool {
+ func_exists(&self.new_name)
+ }
+}
+
#[derive(Clone, Debug)]
pub struct CreateProcParams {
pub name: String,
@@ -1208,17 +1227,21 @@ pub struct CreateProcParams {
pub owner: UserId,
}
+fn func_exists(name: &str) -> bool {
+ let func_space = Space::from(SystemSpace::Func);
+
+ let name_idx = func_space
+ .index_cached("name")
+ .expect("_function should have an index by name");
+ let t = name_idx
+ .get(&[name])
+ .expect("reading from _function shouldn't fail");
+ t.is_some()
+}
+
impl CreateProcParams {
pub fn func_exists(&self) -> bool {
- let func_space = Space::from(SystemSpace::Func);
-
- let name_idx = func_space
- .index_cached("name")
- .expect("_function should have an index by name");
- let t = name_idx
- .get(&[&self.name])
- .expect("reading from _function shouldn't fail");
- t.is_some()
+ func_exists(&self.name)
}
pub fn validate(&self, storage: &Clusterwide) -> traft::Result<()> {
diff --git a/src/sql.rs b/src/sql.rs
index cd5e2c7d60dfc7c7223f469c28fe04bd60659bc1..f90b426a153dfb3b48a039cbb5bb32d4160506aa 100644
--- a/src/sql.rs
+++ b/src/sql.rs
@@ -2,8 +2,9 @@
use crate::schema::{
wait_for_ddl_commit, CreateProcParams, CreateTableParams, DistributionParam, Field,
- PrivilegeDef, PrivilegeType, RoleDef, RoutineDef, RoutineLanguage, RoutineParamDef,
- RoutineParams, RoutineSecurity, SchemaObjectType, ShardingFn, UserDef, ADMIN_ID,
+ PrivilegeDef, PrivilegeType, RenameRoutineParams, RoleDef, RoutineDef, RoutineLanguage,
+ RoutineParamDef, RoutineParams, RoutineSecurity, SchemaObjectType, ShardingFn, UserDef,
+ ADMIN_ID,
};
use crate::sql::pgproto::{
with_portals, BoxedPortal, Describe, Descriptor, UserDescriptors, PG_PORTALS,
@@ -914,6 +915,19 @@ fn reenterable_schema_change_request(
// Nothing to check
Params::DropTable(name)
}
+ IrNode::Ddl(Ddl::RenameRoutine {
+ old_name,
+ new_name,
+ params,
+ ..
+ }) => {
+ let params = RenameRoutineParams {
+ new_name,
+ old_name,
+ params,
+ };
+ Params::RenameRoutine(params)
+ }
IrNode::Acl(Acl::DropUser { name, .. }) => {
// Nothing to check
Params::DropUser(name)
@@ -1039,6 +1053,42 @@ fn reenterable_schema_change_request(
ddl,
}
}
+ Params::RenameRoutine(params) => {
+ if !params.func_exists() {
+ // Procedure does not exist, nothing to rename
+ return Ok(ConsumerResult { row_count: 0 });
+ }
+
+ if params.new_name_occupied() {
+ return Err(Error::Other(
+ format!("Name '{}' is already taken", params.new_name).into(),
+ ));
+ }
+
+ let routine_def = node
+ .storage
+ .routines
+ .by_name(¶ms.old_name)?
+ .expect("if routine ddl is correct, routine must exist");
+
+ if let Some(params) = params.params.as_ref() {
+ ensure_parameters_match(&routine_def, params)?;
+ }
+
+ let ddl = OpDdl::RenameProcedure {
+ routine_id: routine_def.id,
+ new_name: params.new_name.clone(),
+ old_name: params.old_name.clone(),
+ initiator_id: current_user,
+ owner_id: routine_def.owner,
+ schema_version,
+ };
+
+ Op::DdlPrepare {
+ ddl,
+ schema_version,
+ }
+ }
Params::CreateTable(params) => {
if params.space_exists()? {
// Space already exists, no op needed
@@ -1311,6 +1361,7 @@ fn reenterable_schema_change_request(
DropRole(String),
GrantPrivilege(GrantRevokeType, String),
RevokePrivilege(GrantRevokeType, String),
+ RenameRoutine(RenameRoutineParams),
CreateProcedure(CreateProcParams),
DropProcedure(String, Vec<ParamDef>),
}
diff --git a/src/sql/pgproto.rs b/src/sql/pgproto.rs
index c20fdc76c1bbf7cb8f3bc28c4c621594982181ed..5f142990db6e516e1ed494ffd1729ad38366f156 100644
--- a/src/sql/pgproto.rs
+++ b/src/sql/pgproto.rs
@@ -255,6 +255,7 @@ pub enum CommandTag {
Grant = 7,
GrantRole = 8,
Insert = 9,
+ RenameRoutine = 17,
Revoke = 10,
RevokeRole = 11,
#[default]
@@ -275,6 +276,7 @@ impl From<CommandTag> for QueryType {
CommandTag::DropTable
| CommandTag::CreateTable
| CommandTag::CreateProcedure
+ | CommandTag::RenameRoutine
| CommandTag::DropProcedure => QueryType::Ddl,
CommandTag::Delete
| CommandTag::Insert
@@ -312,6 +314,7 @@ impl TryFrom<&Node> for CommandTag {
Ddl::CreateTable { .. } => Ok(CommandTag::CreateTable),
Ddl::CreateProc { .. } => Ok(CommandTag::CreateProcedure),
Ddl::DropProc { .. } => Ok(CommandTag::DropProcedure),
+ Ddl::RenameRoutine { .. } => Ok(CommandTag::RenameRoutine),
},
Node::Relational(rel) => match rel {
Relational::Delete { .. } => Ok(CommandTag::Delete),
diff --git a/src/storage.rs b/src/storage.rs
index c1afd7df6b924755b57587067ba018b51e599f25..9487e5766b7ad1205fbc460ea9f15295bc573ceb 100644
--- a/src/storage.rs
+++ b/src/storage.rs
@@ -2356,6 +2356,27 @@ pub fn ddl_abort_on_master(ddl: &Ddl, version: u64) -> traft::Result<()> {
// Actual drop happens only on commit, so there's nothing to abort.
}
+ Ddl::RenameProcedure {
+ routine_id,
+ ref old_name,
+ ref new_name,
+ ..
+ } => {
+ let lua = ::tarantool::lua_state();
+ lua.exec_with(
+ "local new_name = ...
+ box.schema.func.drop(new_name, {if_exists = true})
+ ",
+ (new_name,),
+ )
+ .map_err(LuaError::from)?;
+
+ lua.exec_with(LUA_CREATE_PROC_SCRIPT, (routine_id, old_name, true))
+ .map_err(LuaError::from)?;
+
+ set_local_schema_version(version)?;
+ }
+
_ => {
todo!();
}
@@ -2364,29 +2385,50 @@ pub fn ddl_abort_on_master(ddl: &Ddl, version: u64) -> traft::Result<()> {
Ok(())
}
+// This script is reused between create and rename procedure,
+// when applying changes on master.
+const LUA_CREATE_PROC_SCRIPT: &str = r#"
+ local func_id, func_name, not_exists = ...
+ local def = {
+ language = 'LUA',
+ body = string.format(
+ [[function() error("function %s is used internally by picodata") end]],
+ func_name
+ ),
+ id = func_id,
+ if_not_exists = not_exists,
+ }
+ box.schema.func.create(func_name, def)"#;
+
/// Create tarantool function which throws an error if it's called.
/// Tarantool function is created with `if_not_exists = true`, so it's
/// safe to call this rust function multiple times.
pub fn ddl_create_function_on_master(func_id: u32, func_name: &str) -> traft::Result<()> {
debug_assert!(unsafe { tarantool::ffi::tarantool::box_txn() });
let lua = ::tarantool::lua_state();
+ lua.exec_with(LUA_CREATE_PROC_SCRIPT, (func_id, func_name, true))
+ .map_err(LuaError::from)?;
+ Ok(())
+}
+
+pub fn ddl_rename_function_on_master(id: u32, old_name: &str, new_name: &str) -> traft::Result<()> {
+ debug_assert!(unsafe { tarantool::ffi::tarantool::box_txn() });
+
+ // We can't do update/replace on _func space, so in order to rename
+ // our dummy lua function we need to drop it and create it with a
+ // new name.
+ let lua = ::tarantool::lua_state();
lua.exec_with(
- r#"
- local func_id, func_name = ...
- local def = {
- language = 'LUA',
- body = string.format(
- [[function() error("function %s is used internally by picodata") end]],
- func_name
- ),
- id = func_id,
- if_not_exists = true,
- }
- box.schema.func.create(func_name, def)
- "#,
- (func_id, func_name),
+ "local func_name = ...
+ box.schema.func.drop(func_name, {if_exists = true})
+ ",
+ (old_name,),
)
.map_err(LuaError::from)?;
+
+ lua.exec_with(LUA_CREATE_PROC_SCRIPT, (id, new_name, false))
+ .map_err(LuaError::from)?;
+
Ok(())
}
@@ -3127,6 +3169,14 @@ impl Routines {
]
}
+ #[inline(always)]
+ pub fn rename(&self, id: u32, new_name: &str) -> tarantool::Result<Option<RoutineDef>> {
+ let mut ops = UpdateOps::with_capacity(1);
+ ops.assign(RoutineDef::FIELD_NAME, new_name)?;
+ let tuple = self.space.update(&[id], ops)?;
+ tuple.as_ref().map(Tuple::decode).transpose()
+ }
+
#[inline(always)]
pub fn by_name(&self, routine_name: &str) -> tarantool::Result<Option<RoutineDef>> {
let tuple = self.index_name.get(&[routine_name])?;
diff --git a/src/traft/node.rs b/src/traft/node.rs
index 37b2fba3c8e435f43dc075a2989677cbfc75824f..b70587d6fcb8a796fa96b9d1a913a07d2b3b23cd 100644
--- a/src/traft/node.rs
+++ b/src/traft/node.rs
@@ -1027,6 +1027,26 @@ impl NodeImpl {
initiator: initiator_def.name,
);
}
+ Ddl::RenameProcedure {
+ routine_id,
+ old_name,
+ new_name,
+ initiator_id,
+ ..
+ } => {
+ self.storage
+ .routines
+ .update_operable(routine_id, true)
+ .expect("storage shouldn't fail");
+
+ let initiator_def = user_by_id(initiator_id).expect("user must exist");
+ crate::audit!(
+ message: "renamed routine {routine_id} from {old_name} to {new_name}",
+ title: "rename routine",
+ severity: Medium,
+ initiator: initiator_def.name,
+ );
+ }
_ => {
todo!()
}
@@ -1093,6 +1113,21 @@ impl NodeImpl {
.expect("storage shouldn't fail");
}
+ Ddl::RenameProcedure {
+ routine_id,
+ old_name,
+ ..
+ } => {
+ self.storage
+ .routines
+ .rename(routine_id, old_name.as_str())
+ .expect("storage shouldn't fail");
+ self.storage
+ .routines
+ .update_operable(routine_id, true)
+ .expect("storage shouldn't fail");
+ }
+
_ => {
todo!()
}
@@ -1412,6 +1447,21 @@ impl NodeImpl {
.update_operable(id, false)
.expect("storage shouldn't fail");
}
+ Ddl::RenameProcedure {
+ routine_id,
+ new_name,
+ ..
+ } => {
+ self.storage
+ .routines
+ .rename(routine_id, new_name.as_str())?
+ .expect("routine existence must have been checked before commit");
+
+ self.storage
+ .routines
+ .update_operable(routine_id, false)
+ .expect("storage shouldn't fail");
+ }
}
self.storage
diff --git a/src/traft/op.rs b/src/traft/op.rs
index e7729814b150a73cd606deb56a37d3087ad65088..e1fe9ebb38b6848e3cd5e8d2a4c1044fcce6a182 100644
--- a/src/traft/op.rs
+++ b/src/traft/op.rs
@@ -120,6 +120,21 @@ impl std::fmt::Display for Op {
} => {
write!(f, "DdlPrepare({schema_version}, DropProcedure({id}))")
}
+ Self::DdlPrepare {
+ schema_version,
+ ddl:
+ Ddl::RenameProcedure {
+ routine_id,
+ old_name,
+ new_name,
+ ..
+ },
+ } => {
+ write!(
+ f,
+ "DdlPrepare({schema_version}, CreateProcedure({routine_id}, {old_name} -> {new_name}))"
+ )
+ }
Self::DdlCommit => write!(f, "DdlCommit"),
Self::DdlAbort => write!(f, "DdlAbort"),
Self::Acl(Acl::CreateUser { user_def }) => {
@@ -535,6 +550,14 @@ pub enum Ddl {
id: RoutineId,
initiator: UserId,
},
+ RenameProcedure {
+ routine_id: u32,
+ old_name: String,
+ new_name: String,
+ initiator_id: UserId,
+ owner_id: UserId,
+ schema_version: u64,
+ },
}
/// Builder for [`Op::DdlPrepare`] operations.
diff --git a/test/int/test_sql.py b/test/int/test_sql.py
index 7ce007485115e19930ad999c166ccec7c8349d53..378abc3923d0c6f83cd179821485ecaf2416e51c 100644
--- a/test/int/test_sql.py
+++ b/test/int/test_sql.py
@@ -2297,6 +2297,7 @@ def test_create_drop_procedure(cluster: Cluster):
create procedure proc1(int, text)
language SQL
as $$insert into t values(?, ?)$$
+ option(timeout=3)
"""
)
with pytest.raises(
@@ -2568,3 +2569,134 @@ def test_call_procedure(cluster: Cluster):
ReturnError, match="AccessDenied: Execute access to function 'proc1' is denied"
):
i1.sql(""" call "proc1"(3, 3) """, user=username, password=alice_pwd)
+
+
+def test_rename_procedure(cluster: Cluster):
+ i1, i2 = cluster.deploy(instance_count=2)
+
+ data = i1.sql(
+ """
+ create table t (a int not null, b int, primary key (a))
+ using memtx
+ distributed by (b)
+ """
+ )
+ assert data["row_count"] == 1
+
+ data = i2.sql(
+ """
+ create procedure foo(int)
+ language SQL
+ as $$insert into t values(?, ?)$$
+ """
+ )
+ assert data["row_count"] == 1
+
+ data = i1.sql(
+ """
+ alter procedure foo
+ rename to bar
+ option ( timeout = 4 )
+ """
+ )
+ assert data["row_count"] == 1
+
+ data = i2.sql(
+ """
+ select "name" from "_pico_routine"
+ where "name" = 'BAR' or "name" = 'FOO'
+ """
+ )
+ assert data["rows"] == [["BAR"]]
+
+ # procedure foo doesn't exist
+ data = i1.sql(
+ """
+ alter procedure foo
+ rename to bar
+ """
+ )
+ assert data["row_count"] == 0
+
+ # rename back, use syntax with parameters
+ data = i1.sql(
+ """
+ alter procedure "BAR"(int)
+ rename to "FOO"
+ """
+ )
+ assert data["row_count"] == 1
+
+ data = i2.sql(
+ """
+ select "name" from "_pico_routine"
+ where "name" = 'BAR' or "name" = 'FOO'
+ """
+ )
+ assert data["rows"] == [["FOO"]]
+
+ data = i2.sql(
+ """
+ create procedure bar()
+ language SQL
+ as $$insert into t values(200, 200)$$
+ """
+ )
+ assert data["row_count"] == 1
+
+ with pytest.raises(ReturnError, match="Name 'FOO' is already taken"):
+ data = i1.sql(
+ """
+ alter procedure bar()
+ rename to foo
+ """
+ )
+
+ with pytest.raises(
+ ReturnError, match="routine exists but with a different signature: BAR()"
+ ):
+ data = i1.sql(
+ """
+ alter procedure bar(int)
+ rename to buzz
+ """
+ )
+
+ username = "alice"
+ alice_pwd = "Passw0rd"
+ acl = i1.sql(
+ f"""
+ create user "{username}" with password '{alice_pwd}'
+ using chap-sha1 option (timeout = 3)
+ """
+ )
+ assert acl["row_count"] == 1
+ with pytest.raises(
+ ReturnError, match="Alter access to function 'BAR' is denied for user 'alice'"
+ ):
+ i1.sql(
+ """alter procedure bar rename to buzz""", user=username, password=alice_pwd
+ )
+
+ i2.eval(
+ """
+ box.schema.func.create(
+ 'foobar',
+ {
+ body = [[function(question) return 42 end]],
+ id = box.internal.generate_func_id(true)
+ }
+ )
+ """
+ )
+ with pytest.raises(ReturnError, match="ddl operation was aborted"):
+ i1.sql(
+ """
+ alter procEdurE foo rENaME tO "foobar"
+ """
+ )
+
+ data = i1.sql(""" select * from "_pico_routine" where "name" = 'foobar' """)
+ assert data["rows"] == []
+ data = i2.sql(""" select * from "_pico_routine" where "name" = 'foobar' """)
+ assert data["rows"] == []