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.

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

Supports drop schema/database statement #825

Closed
killme2008 opened this issue Jan 4, 2023 · 12 comments
Closed

Supports drop schema/database statement #825

killme2008 opened this issue Jan 4, 2023 · 12 comments
Assignees
Labels
C-feature Category Features
Milestone

Comments

@killme2008
Copy link
Contributor

What problem does the new feature solve?

The greptimedb doesn't support drop [schema | database] [name] right now:

CREATE SCHEMA test;
Affected Rows: 1

DROP SCHEMA test;

Failed to execute, error: Datanode { code: 1001, msg: "Failed to execute sql, source: Failure during query execution, source: Cannot parse SQL, source: SQL statement is not supported:  DROP SCHEMA test;, keyword: SCHEMA" }

What does the feature do?

Drop schema(database) by sql statement.

Implementation challenges

No response

@killme2008 killme2008 added C-feature Category Features good first issue Good for newcomers labels Jan 4, 2023
@TheR1sing3un
Copy link

Please assign it to me if you are willing to~

@MichaelScofield
Copy link
Collaborator

ping @TheR1sing3un , any updates?

@NiwakaDev
Copy link
Collaborator

Is this issue blocked until any issues are resolved? If not, I'd like to work on this issue.

@waynexia
Copy link
Member

waynexia commented Jul 7, 2023

Appreciate it ❤️

@NiwakaDev
Copy link
Collaborator

NiwakaDev commented Jul 10, 2023

Next I'll implement DropDatabaseProcedure. What do you think about this?

@NiwakaDev
Copy link
Collaborator

NiwakaDev commented Jul 10, 2023

I have a question about the design of the lock_key return value in the DropDatabaseProcedure. Since the DropDatabaseProcedure deletes all tables on a schema, it needs to block all operations on the schema performed by other procedures. As far as I understand, I'd like to add the schema_name to the lock_key return value for CreateTableProcedure, AlterTableProcedure, and DropTableProcedure. Here's an example of adding schema_name to LockKey in CreateTableProcedure:

#[async_trait]
impl Procedure for CreateTableProcedure {
    //..
    fn lock_key(&self) -> LockKey {
        // We lock the whole table and the schema.
        let table_name = self.data.table_ref().to_string();
        LockKey::new(vec![table_name, self.request.schema_name.clone()])
    }
}

Similarly, for DropDatabaseProcedure, we also need to lock the schema_name, like so:

#[async_trait]
impl Procedure for DropDatabaseProcedure {
    //..

    fn lock_key(&self) -> LockKey {
        LockKey::new([
            self.request.schema_name.clone(),
            //..
        ])
    }
}

I think it is inefficient to lock the schema. Are there other effective approaches?

@evenyag
Copy link
Contributor

evenyag commented Jul 11, 2023

I think it is inefficient to lock the schema.

Yes, we don't support hierarchy locking now. We could provide an intuitive implementation first:

  • lists all tables under the schema
  • locks tables listed
  • drops those tables
  • checks if new tables are added to the schema, if so, we abort the procedure and signal the procedure is failed

This means that a create table command aborts the drop database command. But users could fix this by re-submit a drop database command.

In the future, we can improve this by other approaches. There are some alternatives:

  • maintains states for each database in memory and checks this state before creating/dropping tables
  • support hierarchy locking

@evenyag
Copy link
Contributor

evenyag commented Sep 6, 2023

We might choose another approach, see #1943 (comment)

@WenyXu
Copy link
Member

WenyXu commented Jan 3, 2024

@NiwakaDev After #3061, we allow the procedure to acquire a shared lock, which makes acquiring locks in the drop database procedure much more straightforward. I will refactor the locks of procedures recently to ensure we only need to acquire an exclusive lock on the database key during the drop database procedure.

@fengjiachun fengjiachun modified the milestones: v0.6, v0.8 Feb 28, 2024
@WenyXu
Copy link
Member

WenyXu commented Mar 11, 2024

@NiwakaDev, are there any updates? Would you like to continue working on this feature, Or may I take over the feature?

@tisonkun
Copy link
Collaborator

@WenyXu drop database is now tracked by #3516. Is it simliar to drop schema? I wonder if we can supersede this issue with other recent issues to keep the context updated.

@WenyXu
Copy link
Member

WenyXu commented May 6, 2024

@WenyXu drop database is now tracked by #3516. Is it simliar to drop schema? I wonder if we can supersede this issue with other recent issues to keep the context updated.

It's the same thing. Let's close it

@WenyXu WenyXu closed this as completed May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature Category Features
Projects
None yet
Development

No branches or pull requests

9 participants