Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

impl alter database rename #4984

Closed
wants to merge 1 commit into from
Closed

impl alter database rename #4984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions common/meta/api/src/meta_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ use common_meta_types::ListDatabaseReq;
use common_meta_types::ListTableReq;
use common_meta_types::MetaError;
use common_meta_types::MetaId;
use common_meta_types::RenameDatabaseReply;
use common_meta_types::RenameDatabaseReq;
use common_meta_types::RenameTableReply;
use common_meta_types::RenameTableReq;
use common_meta_types::ShareInfo;
Expand Down Expand Up @@ -62,6 +64,11 @@ pub trait MetaApi: Send + Sync {
req: ListDatabaseReq,
) -> Result<Vec<Arc<DatabaseInfo>>, MetaError>;

async fn rename_database(
&self,
req: RenameDatabaseReq,
) -> Result<RenameDatabaseReply, MetaError>;

// table

async fn create_table(&self, req: CreateTableReq) -> Result<CreateTableReply, MetaError>;
Expand Down
122 changes: 122 additions & 0 deletions common/meta/api/src/meta_api_test_suite.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use common_meta_types::GetShareReq;
use common_meta_types::GetTableReq;
use common_meta_types::ListDatabaseReq;
use common_meta_types::ListTableReq;
use common_meta_types::RenameDatabaseReq;
use common_meta_types::RenameTableReq;
use common_meta_types::TableIdent;
use common_meta_types::TableInfo;
Expand Down Expand Up @@ -385,6 +386,127 @@ impl MetaApiTestSuite {
Ok(())
}

pub async fn database_rename<MT: MetaApi>(self, mt: &MT) -> anyhow::Result<()> {
let tenant = "tenant1";
let db_name = "db1";
let db2_name = "db2";
let new_db_name = "db3";

tracing::info!("--- rename not exists db1 to not exists db2");
{
let req = RenameDatabaseReq {
if_exists: false,
tenant: tenant.to_string(),
db_name: db_name.to_string(),
new_db_name: new_db_name.to_string(),
};

let res = mt.rename_database(req).await;
tracing::info!("rename database res: {:?}", res);
assert!(res.is_err());
assert_eq!(
ErrorCode::UnknownDatabase("").code(),
ErrorCode::from(res.unwrap_err()).code()
);
}

tracing::info!("--- prepare db1 and db2");
{
// prepare db2
let res = self.create_database(mt, tenant, "db1").await?;
assert_eq!(1, res.database_id);

tracing::info!("--- rename not exists db4 to exists db1");
{
let req = RenameDatabaseReq {
if_exists: false,
tenant: tenant.to_string(),
db_name: "db4".to_string(),
new_db_name: db_name.to_string(),
};

let res = mt.rename_database(req).await;
tracing::info!("rename database res: {:?}", res);
assert!(res.is_err());
assert_eq!(
ErrorCode::UnknownDatabase("").code(),
ErrorCode::from(res.unwrap_err()).code()
);
}

// prepare db2
let res = self.create_database(mt, tenant, "db2").await?;
assert_eq!(2, res.database_id);
}

tracing::info!("--- rename exists db db1 to exists db db2");
{
let req = RenameDatabaseReq {
if_exists: false,
tenant: tenant.to_string(),
db_name: db_name.to_string(),
new_db_name: db2_name.to_string(),
};

let res = mt.rename_database(req).await;
tracing::info!("rename database res: {:?}", res);
assert!(res.is_err());
assert_eq!(
ErrorCode::DatabaseAlreadyExists("").code(),
ErrorCode::from(res.unwrap_err()).code()
);
}

tracing::info!("--- rename exists db db1 to not exists mutable db");
{
let req = RenameDatabaseReq {
if_exists: false,
tenant: tenant.to_string(),
db_name: db_name.to_string(),
new_db_name: new_db_name.to_string(),
};
let res = mt.rename_database(req).await;
tracing::info!("rename database res: {:?}", res);
assert!(res.is_ok());

let res = mt
.get_database(GetDatabaseReq::new(tenant, new_db_name))
.await;
tracing::debug!("get present database res: {:?}", res);
let res = res?;
assert_eq!(1, res.database_id, "db3 id is 1");
assert_eq!("db3".to_string(), res.db, "db3.db is db3");

tracing::info!("--- get old database after rename");
{
let res = mt.get_database(GetDatabaseReq::new(tenant, db_name)).await;
let err = res.err().unwrap();
assert_eq!(
ErrorCode::UnknownDatabase("").code(),
ErrorCode::from(err).code()
);
}
}

tracing::info!("--- drop db2 and db1");
{
mt.drop_database(DropDatabaseReq {
if_exists: false,
tenant: tenant.to_string(),
db_name: db2_name.to_string(),
})
.await?;
mt.drop_database(DropDatabaseReq {
if_exists: false,
tenant: tenant.to_string(),
db_name: new_db_name.to_string(),
})
.await?;
}

Ok(())
}

pub async fn table_create_get_drop<MT: MetaApi>(&self, mt: &MT) -> anyhow::Result<()> {
let tenant = "tenant1";
let db_name = "db1";
Expand Down
11 changes: 11 additions & 0 deletions common/meta/embedded/src/meta_api_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ use common_meta_types::ListDatabaseReq;
use common_meta_types::ListTableReq;
use common_meta_types::MetaError;
use common_meta_types::MetaId;
use common_meta_types::RenameDatabaseReply;
use common_meta_types::RenameDatabaseReq;
use common_meta_types::RenameTableReply;
use common_meta_types::RenameTableReq;
use common_meta_types::ShareInfo;
Expand Down Expand Up @@ -79,6 +81,15 @@ impl MetaApi for MetaEmbedded {
Ok(reply)
}

async fn rename_database(
&self,
req: RenameDatabaseReq,
) -> Result<RenameDatabaseReply, MetaError> {
let sm = self.inner.lock().await;
let reply = sm.rename_database(req).await?;
Ok(reply)
}

async fn create_table(&self, req: CreateTableReq) -> Result<CreateTableReply, MetaError> {
let sm = self.inner.lock().await;
let reply = sm.create_table(req).await?;
Expand Down
8 changes: 7 additions & 1 deletion common/meta/embedded/tests/it/meta_api_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@ async fn test_meta_embedded_database_list_in_diff_tenant() -> anyhow::Result<()>
MetaApiTestSuite {}.database_list_in_diff_tenant(&mt).await
}

#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
async fn test_meta_embedded_database_rename() -> anyhow::Result<()> {
let mt = MetaEmbedded::new_temp().await?;
MetaApiTestSuite {}.database_rename(&mt).await
}

#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
async fn test_meta_embedded_table_create_get_drop() -> anyhow::Result<()> {
let mt = MetaEmbedded::new_temp().await?;
Expand All @@ -61,7 +67,7 @@ async fn test_meta_embedded_table_list() -> anyhow::Result<()> {
}

#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
async fn test_meta_embedded_share_create_get_dro() -> anyhow::Result<()> {
async fn test_meta_embedded_share_create_get_drop() -> anyhow::Result<()> {
let mt = MetaEmbedded::new_temp().await?;
MetaApiTestSuite {}.share_create_get_drop(&mt).await
}
9 changes: 9 additions & 0 deletions common/meta/grpc/src/grpc_action.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ use common_meta_types::ListTableReq;
use common_meta_types::MGetKVActionReply;
use common_meta_types::MetaId;
use common_meta_types::PrefixListReply;
use common_meta_types::RenameDatabaseReply;
use common_meta_types::RenameDatabaseReq;
use common_meta_types::RenameTableReply;
use common_meta_types::RenameTableReq;
use common_meta_types::ShareInfo;
Expand All @@ -58,6 +60,7 @@ pub trait RequestFor {
pub enum MetaGrpcWriteReq {
CreateDatabase(CreateDatabaseReq),
DropDatabase(DropDatabaseReq),
RenameDatabase(RenameDatabaseReq),

CreateTable(CreateTableReq),
DropTable(DropTableReq),
Expand Down Expand Up @@ -199,6 +202,12 @@ impl RequestFor for DropDatabaseReq {
type Reply = DropDatabaseReply;
}

impl RequestFor for RenameDatabaseReq {
type Reply = RenameDatabaseReply;
}

// == table actions ==

impl RequestFor for CreateTableReq {
type Reply = CreateTableReply;
}
Expand Down
9 changes: 9 additions & 0 deletions common/meta/grpc/src/meta_api_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ use common_meta_types::ListDatabaseReq;
use common_meta_types::ListTableReq;
use common_meta_types::MetaError;
use common_meta_types::MetaId;
use common_meta_types::RenameDatabaseReply;
use common_meta_types::RenameDatabaseReq;
use common_meta_types::RenameTableReply;
use common_meta_types::RenameTableReq;
use common_meta_types::ShareInfo;
Expand Down Expand Up @@ -71,6 +73,13 @@ impl MetaApi for MetaGrpcClient {
self.do_read(req).await
}

async fn rename_database(
&self,
req: RenameDatabaseReq,
) -> Result<RenameDatabaseReply, MetaError> {
self.do_write(req).await
}

async fn create_table(&self, req: CreateTableReq) -> Result<CreateTableReply, MetaError> {
self.do_write(req).await
}
Expand Down
108 changes: 108 additions & 0 deletions common/meta/raft-store/src/state_machine/sm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ use common_meta_types::Cmd;
use common_meta_types::CreateDatabaseReq;
use common_meta_types::CreateShareReq;
use common_meta_types::CreateTableReq;
use common_meta_types::DatabaseAlreadyExists;
use common_meta_types::DatabaseMeta;
use common_meta_types::DropDatabaseReq;
use common_meta_types::DropShareReq;
Expand All @@ -50,6 +51,7 @@ use common_meta_types::MetaStorageResult;
use common_meta_types::Node;
use common_meta_types::NodeId;
use common_meta_types::Operation;
use common_meta_types::RenameDatabaseReq;
use common_meta_types::RenameTableReq;
use common_meta_types::SeqV;
use common_meta_types::ShareInfo;
Expand Down Expand Up @@ -451,6 +453,110 @@ impl StateMachine {
Ok(AppliedState::DatabaseMeta(Change::new(None, None)))
}

fn apply_rename_database_cmd(
Copy link
Contributor

@junnplus junnplus Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we reuse apply_drop_database_cmd and apply_create_database_cmd code? Like apply_rename_table_cmd.

Copy link
Collaborator Author

@TCeason TCeason Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we reuse apply_drop_database_cmd and apply_create_database_cmd code? Like apply_rename_table_cmd.

I think renamed database should use old database's db_id.

create database old;
alter database old rename to new

not equal with:

create database old;
drop database old;
create database new;

But maybe we can try to extract the same part.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think renamed database should use old database's db_id.

I'm not sure, it seems that the id and name are bound now.

cc @drmingdrmer @ariesdevil

Copy link
Collaborator Author

@TCeason TCeason Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, actually in apply_rename_database_cmd

https://github.com/datafuselabs/databend/blob/a21fb5bb7da6579c0a80a5d2ead3d8f073b33e77/common/meta/raft-store/src/state_machine/sm.rs#L622

table_id is table_name's id and table_meta also from old table( &prev.as_ref().unwrap().data).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think renamed database should use old database's db_id.

I'm not sure, it seems that the id and name are bound now.

cc @drmingdrmer @ariesdevil

The db_id and table_id need to remain the same when changing the db/table meta

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The db_id and table_id need to remain the same when changing the db/table meta

Thanks, I found it #4838

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TCeason It looks like there is no conflict between reusing the code and not changing the db_id, It can also be refactored in another PR.

Copy link
Collaborator Author

@TCeason TCeason Apr 21, 2022 •
edited by junnplus
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TCeason It looks like there is no conflict between reusing the code and not changing the db_id, It can also be refactored in another PR.

Agree with you.

&self,
req: &RenameDatabaseReq,
txn_tree: &TransactionSledTree,
) -> MetaStorageResult<AppliedState> {
let tenant = &req.tenant;
let name = &req.db_name;
let new_name = &req.new_db_name;
let database_lookup = txn_tree.key_space::<DatabaseLookup>();
let db_key = DatabaseLookupKey::new(tenant.to_string(), name.to_string());
let (prev, result) = self.txn_sub_tree_upsert(
&database_lookup,
&db_key,
&MatchSeq::Any,
Operation::Delete,
None,
)?;

assert!(
result.is_none(),
"rename with MatchSeq::Any always succeeds"
);

// db_name exists
if let Some(seq_db_id) = prev {
let db_id = seq_db_id.data;

let dbs = txn_tree.key_space::<Databases>();
let (prev_meta, result_meta) =
self.txn_sub_tree_upsert(&dbs, &db_id, &MatchSeq::Any, Operation::Delete, None)?;
if prev_meta.is_none() {
return Err(MetaStorageError::AppError(AppError::UnknownDatabase(
UnknownDatabase::new(&req.db_name, "apply_rename_database_cmd"),
)));
}
assert!(result_meta.is_none());
let meta = &prev_meta.as_ref().unwrap().data;

let db_lookup_tree = txn_tree.key_space::<DatabaseLookup>();
let db_key = DatabaseLookupKey::new(tenant.to_string(), new_name.to_string());
let (prev, result) = self.txn_sub_tree_upsert(
&db_lookup_tree,
&db_key,
&MatchSeq::Exact(0),
Operation::Update(db_id),
None,
)?;

// if it is just created
if prev.is_none() && result.is_some() {
// TODO(xp): reconsider this impl. it may not be required.
self.txn_incr_seq(SEQ_DATABASE_META_ID, txn_tree)?;
} else {
// exist
return Err(MetaStorageError::AppError(AppError::DatabaseAlreadyExists(
DatabaseAlreadyExists::new(&req.new_db_name, "apply_rename_database_cmd"),
)));
}

let dbs = txn_tree.key_space::<Databases>();
let (prev_meta, result_meta) = self.txn_sub_tree_upsert(
&dbs,
&db_id,
&MatchSeq::Exact(0),
Operation::Update(meta.clone()),
None,
)?;
if prev_meta.is_some() {
return Err(MetaStorageError::AppError(AppError::DatabaseAlreadyExists(
DatabaseAlreadyExists::new(&req.new_db_name, "apply_rename_database_cmd"),
)));
}
assert!(result_meta.is_some());
if prev_meta.is_none() && result_meta.is_some() {
self.txn_incr_seq(SEQ_DATABASE_META_ID, txn_tree)?;
}

tracing::debug!(
"applied rename Database: from {} to {}, db_id: {}, meta: {:?}",
name,
new_name,
db_id,
result_meta
);

return Ok(AppliedState::DatabaseMeta(Change::new_with_id(
db_id,
prev_meta,
result_meta,
)));
}

// db_name not exist
tracing::debug!(
"applied rename Database: from {} to {}, {:?}",
name,
new_name,
result
);
Err(MetaStorageError::AppError(AppError::UnknownDatabase(
UnknownDatabase::new(&req.db_name, "apply_rename_database_cmd"),
)))
}

#[tracing::instrument(level = "debug", skip(self, txn_tree))]
fn apply_create_table_cmd(
&self,
Expand Down Expand Up @@ -637,6 +743,8 @@ impl StateMachine {

Cmd::DropDatabase(req) => self.apply_drop_database_cmd(req, txn_tree),

Cmd::RenameDatabase(req) => self.apply_rename_database_cmd(req, txn_tree),

Cmd::CreateTable(req) => self.apply_create_table_cmd(req, txn_tree),

Cmd::DropTable(req) => self.apply_drop_table_cmd(req, txn_tree),
Expand Down
Loading