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

feat: drop if exists #2859

Merged
merged 3 commits into from
Dec 5, 2023
Merged

feat: drop if exists #2859

merged 3 commits into from
Dec 5, 2023

Conversation

tisonkun
Copy link
Collaborator

@tisonkun tisonkun commented Dec 2, 2023

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

This closes #2794.

@zuston it happens that I have some time to review this issue, if you're working on this issue, feel free to take any value from my PR and prioritize your patch.

@sunng87 I noticed that we actually check table existence before submit DropTable DDL task. If so, we can simply filter drop table tasks in StatemenExecutor::drop_table and assert table exists for the following producer.

Also, I'd like to confirm whether DDL task procedures can run concurrently. That is, if two DROP TABLE t can run concurrently with t exists. If so, we may need some lock/sync when removing table metadata.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

Copy link

codecov bot commented Dec 2, 2023

Codecov Report

Merging #2859 (13024a8) into develop (781f242) will decrease coverage by 0.35%.
Report is 10 commits behind head on develop.
The diff coverage is 97.77%.

❗ Current head 13024a8 differs from pull request most recent head 4b288a8. Consider uploading reports for the commit 4b288a8 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2859      +/-   ##
===========================================
- Coverage    84.74%   84.40%   -0.35%     
===========================================
  Files          740      740              
  Lines       115767   115796      +29     
===========================================
- Hits         98105    97735     -370     
- Misses       17662    18061     +399     

@tisonkun
Copy link
Collaborator Author

tisonkun commented Dec 3, 2023

Maybe I should add some e2e tests. But let's first take a look at the impl direction.

Copy link
Member

@WenyXu WenyXu left a comment

Choose a reason for hiding this comment

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

Rest LGTM, Let's add some sqlness tests.

src/common/meta/src/rpc/ddl.rs Show resolved Hide resolved
Signed-off-by: tison <[email protected]>
@tisonkun
Copy link
Collaborator Author

tisonkun commented Dec 4, 2023

Updated. PTAL again.

Copy link
Member

@WenyXu WenyXu left a comment

Choose a reason for hiding this comment

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

🚀

@MichaelScofield MichaelScofield added this pull request to the merge queue Dec 5, 2023
@MichaelScofield
Copy link
Collaborator

Drop table procedures cannot run concurrently because they acquire the same procedure lock key. The key locks on the table name.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 5, 2023
@tisonkun
Copy link
Collaborator Author

tisonkun commented Dec 5, 2023

Is this an unstable case - https://github.com/GreptimeTeam/greptimedb/actions/runs/7095139148/job/19311581914?

Since we pass at the first attempt.

Copy link
Collaborator

@fengjiachun fengjiachun left a comment

Choose a reason for hiding this comment

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

LGTM

@fengjiachun fengjiachun added this pull request to the merge queue Dec 5, 2023
Merged via the queue into GreptimeTeam:develop with commit 7d506b3 Dec 5, 2023
18 checks passed
@tisonkun tisonkun deleted the drop-if-exists branch December 5, 2023 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for "if exists" clause for drop table
4 participants