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 drop database failure #16000

Merged
merged 5 commits into from
May 10, 2024
Merged

Conversation

daviszhen
Copy link
Contributor

@daviszhen daviszhen commented May 10, 2024

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #https://github.com/matrixorigin/MO-Cloud/issues/3206

What this PR does / why we need it:

问题原因:
在drop database 会从mo_table_partitions删除索引信息。过程中会加锁。
加锁过程中,会用tableId 取relation。
取relation时,会遍历CatalogCache中的数据库。但是同一个事务中的数据库已经被删除了。
导致engine.Database取Database对象时,报错。

修改方案:
事务中删除drop database时,会先存入deletedDatabaseMap。
在遍历catalogCache中的数据库时,将已经加入deletedDatabaseMap的数据库名称排除掉。避免报错。

ci regression: https://github.com/matrixorigin/ci-test/actions/runs/9032264260

@daviszhen daviszhen changed the title drop db cp fix drop database failure May 10, 2024
@mergify mergify bot requested a review from sukki37 May 10, 2024 12:30
@mergify mergify bot added the kind/bug Something isn't working label May 10, 2024
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label May 10, 2024
@matrix-meow
Copy link
Contributor

@daviszhen Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that the purpose of the PR is to fix a failure related to dropping a database.

Body:

The body of the pull request provides essential information about the issue being addressed and references the related GitHub issue. It explains the root cause of the problem and the proposed solution, which involves storing deleted databases in a map to prevent errors when iterating through the database cache.

Changes:

  1. The changes in the engine.go file involve modifications to the Databases, GetNameById, and GetRelationById functions to exclude deleted databases when retrieving database information. This is achieved by introducing a new function getDatabasesExceptDeleted and utilizing it to filter out deleted databases from the catalog cache.

  2. The util.go file includes the implementation of getDatabasesExceptDeleted, removeIf, and find functions to handle the exclusion of deleted databases from the cache.

  3. The util_test.go file contains test cases for the removeIf function to ensure its correctness.

Feedback and Suggestions:

  1. Error Handling: It's important to ensure proper error handling throughout the code. Make sure to handle potential errors that may arise during database retrieval or deletion operations.

  2. Code Optimization: Consider optimizing the removeIf function for better performance. Instead of iterating over the entire dataset, you could use a more efficient data structure or algorithm to filter out deleted databases.

  3. Variable Naming: Improve the clarity of variable names for better readability. For example, dbsExceptDelete could be renamed to something more descriptive like filteredDatabases.

  4. Consistency: Ensure consistency in naming conventions and coding style across the codebase to enhance maintainability.

  5. Security Concerns: While the changes seem focused on functionality, it's crucial to consider any potential security implications. Ensure that the modifications do not introduce vulnerabilities such as data leakage or unauthorized access.

  6. Testing: The addition of test cases is commendable. Make sure to cover edge cases and handle different scenarios to validate the correctness of the implemented functions.

  7. Documentation: Consider adding comments or documentation to explain the purpose and usage of newly introduced functions for better code understanding.

By addressing the above points, you can enhance the quality, performance, and security of the codebase while ensuring that the fix for the database drop failure is robust and reliable.

@mergify mergify bot merged commit 1760453 into matrixorigin:1.2-dev May 10, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/S Denotes a PR that changes [10,99] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants