impl alter database rename #5286

TCeason · 2022-05-10T13:53:00Z

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Support alter database.

Changelog

New Feature

Related Issues

Fixes #4839

vercel · 2022-05-10T13:53:06Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
databend	✅ Ready (Inspect)	Visit Preview	May 11, 2022 at 0:20AM (UTC)

mergify · 2022-05-10T13:53:08Z

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

drmingdrmer

A great PR!

drmingdrmer · 2022-05-10T18:08:43Z

common/meta/api/src/schema_api_impl.rs

+                            // TODO(like rename table): if database not exists but sql hint if_exists is true
+                            // the sql must be Ok but should not return database_id
+                            return Ok(RenameDatabaseReply { db_id: 0 });


I'd suggest removing db_id in the RenameDatabaseReply because renaming a db does not change its id.

Ok I will have a look.

drmingdrmer · 2022-05-10T18:11:39Z

common/meta/api/src/schema_api_impl.rs

+            let (db_id_seq, _db_id) = get_id_value(self, &tenant_newdbname).await?;
+            db_has_to_not_exist(db_id_seq, &tenant_newdbname, "rename_database")?;
+
+            let new_db_id = fetch_id(self, DatabaseIdGen {}).await?;


Just use the old_db_id. We do not need to create a new id. So that the application(databend-query) will always be able to access this db with the same id.

drmingdrmer · 2022-05-10T18:13:17Z

common/meta/api/src/schema_api_impl.rs

+            // rename database
+            {
+                let txn_req = TxnRequest {
+                    condition: vec![txn_cond_seq(&tenant_newdbname, Eq, 0)?],


It requires another txn_cond_seq to ensure the (tenant, old_db_name) -> (old_db_id_seq, old_db_id) is not removed before this transaction completes.

common/meta/api/src/schema_api_impl.rs

common/meta/api/src/schema_api_test_suite.rs

common/ast/src/ast/statement.rs

common/meta/api/src/schema_api_impl.rs

BohuTANG · 2022-05-11T12:05:19Z

@mergify update

mergify · 2022-05-11T12:05:39Z

update

✅ Branch has been successfully updated

drmingdrmer · 2022-05-11T12:24:39Z

common/meta/api/src/schema_api_impl.rs

+            let res = get_db_or_err(
+                self,
+                tenant_dbname,
+                format!("rename_database: {}", &tenant_dbname),
+            )
+            .await;
+
+            let (old_db_id_seq, old_db_id, _, _) = match res {
+                Ok(x) => x,
+                Err(e) => {
+                    if let MetaError::AppError(AppError::UnknownDatabase(_)) = e {
+                        if req.if_exists {
+                            return Ok(RenameDatabaseReply {});
+                        }
+                    }
+                    return Err(e);
+                }
+            };


Since db_meta is never used, this part can be simplified to let res = get_id_value(self, tenant_db_name...) ..., which will save one RPC to metasrv.

But it's not a big deal and can be done in next PR.

TCeason requested a review from BohuTANG as a code owner May 10, 2022 13:53

databend-bot added the need-review label May 10, 2022

mergify bot added pr-feature this PR introduces a new feature to the codebase pr-build this PR changes build/testing/ci steps pr-doc-fix labels May 10, 2022

vercel bot deployed to Preview May 10, 2022 13:54 View deployment

TCeason force-pushed the ISSUE-4839/support_alter_database_with_new_api branch from fe3d6bd to 20f2e73 Compare May 10, 2022 14:19

vercel bot deployed to Preview May 10, 2022 14:20 View deployment

TCeason force-pushed the ISSUE-4839/support_alter_database_with_new_api branch from 20f2e73 to 383f32c Compare May 10, 2022 14:30

vercel bot deployed to Preview May 10, 2022 14:31 View deployment

TCeason force-pushed the ISSUE-4839/support_alter_database_with_new_api branch from 383f32c to b83df89 Compare May 10, 2022 14:36

vercel bot deployed to Preview May 10, 2022 14:36 View deployment

BohuTANG requested review from drmingdrmer and removed request for BohuTANG May 10, 2022 15:27

BohuTANG removed pr-build this PR changes build/testing/ci steps pr-doc-fix labels May 10, 2022

mergify bot added pr-build this PR changes build/testing/ci steps pr-doc-fix labels May 10, 2022

TCeason force-pushed the ISSUE-4839/support_alter_database_with_new_api branch from b83df89 to 7edcaef Compare May 10, 2022 15:29

vercel bot deployed to Preview May 10, 2022 15:30 View deployment

drmingdrmer reviewed May 10, 2022

View reviewed changes

TCeason force-pushed the ISSUE-4839/support_alter_database_with_new_api branch from 7edcaef to 142de4f Compare May 11, 2022 08:27

vercel bot deployed to Preview May 11, 2022 08:28 View deployment

andylokandy reviewed May 11, 2022

View reviewed changes

common/ast/src/ast/statement.rs Outdated Show resolved Hide resolved

common/ast/src/ast/statement.rs Outdated Show resolved Hide resolved

TCeason force-pushed the ISSUE-4839/support_alter_database_with_new_api branch from 142de4f to f740c94 Compare May 11, 2022 09:22

vercel bot deployed to Preview May 11, 2022 09:23 View deployment

TCeason removed pr-build this PR changes build/testing/ci steps pr-doc-fix labels May 11, 2022

drmingdrmer reviewed May 11, 2022

View reviewed changes

common/meta/api/src/schema_api_impl.rs Outdated Show resolved Hide resolved

TCeason force-pushed the ISSUE-4839/support_alter_database_with_new_api branch from 9ff69b9 to 9e33858 Compare May 11, 2022 12:18

impl alter database rename

1cdccac

vercel bot deployed to Preview May 11, 2022 12:19 View deployment

TCeason force-pushed the ISSUE-4839/support_alter_database_with_new_api branch from 9e33858 to 1cdccac Compare May 11, 2022 12:19

vercel bot deployed to Preview May 11, 2022 12:20 View deployment

drmingdrmer reviewed May 11, 2022

View reviewed changes

drmingdrmer approved these changes May 11, 2022

View reviewed changes

BohuTANG mentioned this pull request May 11, 2022

Since db_meta is never used, this part can be simplified to let res = get_id_value(self, tenant_db_name...) ..., which will save one RPC to metasrv. #5303

Closed

BohuTANG merged commit 07c1db0 into databendlabs:main May 11, 2022

TCeason deleted the ISSUE-4839/support_alter_database_with_new_api branch June 9, 2022 02:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

impl alter database rename #5286

impl alter database rename #5286

TCeason commented May 10, 2022 •

edited

Loading

vercel bot commented May 10, 2022 •

edited

Loading

mergify bot commented May 10, 2022

drmingdrmer left a comment

drmingdrmer May 10, 2022

TCeason May 11, 2022

drmingdrmer May 10, 2022

drmingdrmer May 10, 2022

BohuTANG commented May 11, 2022

mergify bot commented May 11, 2022

drmingdrmer May 11, 2022

impl alter database rename #5286

impl alter database rename #5286

Conversation

TCeason commented May 10, 2022 • edited Loading

Summary

Changelog

Related Issues

vercel bot commented May 10, 2022 • edited Loading

mergify bot commented May 10, 2022

drmingdrmer left a comment

Choose a reason for hiding this comment

drmingdrmer May 10, 2022

Choose a reason for hiding this comment

TCeason May 11, 2022

Choose a reason for hiding this comment

drmingdrmer May 10, 2022

Choose a reason for hiding this comment

drmingdrmer May 10, 2022

Choose a reason for hiding this comment

BohuTANG commented May 11, 2022

mergify bot commented May 11, 2022

✅ Branch has been successfully updated

drmingdrmer May 11, 2022

Choose a reason for hiding this comment

TCeason commented May 10, 2022 •

edited

Loading

vercel bot commented May 10, 2022 •

edited

Loading