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

fix: concurrent rename two table to same name may cause override #1368

Merged
merged 3 commits into from
Apr 13, 2023

Conversation

v0y4g3r
Copy link
Contributor

@v0y4g3r v0y4g3r commented Apr 12, 2023

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

fix concurrent rename two table to same name may cause override

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)

fixes #1369

@v0y4g3r v0y4g3r marked this pull request as ready for review April 12, 2023 07:59
Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

🫨

@v0y4g3r v0y4g3r enabled auto-merge (squash) April 12, 2023 08:17
Copy link
Contributor

@evenyag evenyag left a comment

Choose a reason for hiding this comment

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

Does the LocalCatalogManager have a similar issue?

let schema = catalog
.schema(schema_name)?
.with_context(|| SchemaNotFoundSnafu {
catalog: catalog_name,
schema: schema_name,
})?;
let old_table = schema
.table(&request.table_name)
.await?
.context(TableNotExistSnafu {
table: &request.table_name,
})?;
let engine = old_table.table_info().meta.engine.to_string();
// rename table in system catalog
self.system
.register_table(
catalog_name.clone(),
schema_name.clone(),
request.new_table_name.clone(),
request.table_id,
engine,
)
.await?;
Ok(schema
.rename_table(&request.table_name, request.new_table_name)
.is_ok())

Looks like we should acquire the register lock and check whether the new table exists

@v0y4g3r v0y4g3r force-pushed the fix/concurrent-rename branch from 272af08 to bd68f2a Compare April 12, 2023 13:16
@v0y4g3r

This comment was marked as resolved.

@codecov
Copy link

codecov bot commented Apr 12, 2023

Codecov Report

Merging #1368 (1c3a499) into develop (ae21c1c) will decrease coverage by 0.28%.
The diff coverage is 65.00%.

@@             Coverage Diff             @@
##           develop    #1368      +/-   ##
===========================================
- Coverage    86.04%   85.76%   -0.28%     
===========================================
  Files          523      528       +5     
  Lines        78396    78486      +90     
===========================================
- Hits         67454    67317     -137     
- Misses       10942    11169     +227     

@v0y4g3r v0y4g3r requested a review from evenyag April 13, 2023 03:23
@v0y4g3r v0y4g3r merged commit 9fa871a into GreptimeTeam:develop Apr 13, 2023
paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
…ptimeTeam#1368)

* fix: concurrent rename two table to same name may cause override

* fix: concurrently update system catalog table

* fix: correctness
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.

Race condition when rename two tables to the same name
3 participants