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

disable pre allocate on uncommitted txn to 1.1 #15104

Merged
merged 2 commits into from
Mar 22, 2024

Conversation

zhangxu19830126
Copy link
Contributor

@zhangxu19830126 zhangxu19830126 commented Mar 22, 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 #15075

What this PR does / why we need it:

disable pre allocate on uncommitted txn to 1.1

@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Mar 22, 2024
@mergify mergify bot requested a review from sukki37 March 22, 2024 03:37
@mergify mergify bot added the kind/bug Something isn't working label Mar 22, 2024
@matrix-meow
Copy link
Contributor

@zhangxu19830126 Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating the specific change being made.

Body:

The body of the pull request provides relevant information about the type of PR and the issue it fixes. It also explains the purpose of the change, which is to disable pre-allocation on uncommitted transactions.

Changes:

  1. In column_cache.go:

    • A new field committed of type bool has been added to the columnCache struct.
    • The newColumnCache function now accepts a committed parameter and initializes the committed field accordingly.
    • The maybeAllocate function now checks if low and committed are both true before proceeding with allocation.
  2. In column_cache_test.go:

    • The test case for newColumnCache has been updated to pass true as the committed parameter.
  3. In table_cache.go:

    • The newTableCache function now accepts a committed parameter and passes it to the newColumnCache function.
    • The commit function now sets the committed flag to true for each column in the table cache.

Suggestions for Improvement:

  1. Security Concern - Lack of Synchronization:

    • The addition of the committed field in the columnCache struct introduces a potential race condition. Accessing and modifying the committed field without proper synchronization can lead to data inconsistency or unexpected behavior.
    • Recommendation: Ensure proper synchronization mechanisms (e.g., mutex locks) are used when reading and writing to the committed field to prevent race conditions.
  2. Code Optimization:

    • Instead of setting the committed flag individually for each column in the tableCache during commit, consider refactoring the code to handle this operation more efficiently.
    • Recommendation: Implement a method in the columnCache struct to handle the commitment status, reducing redundancy and improving code maintainability.
  3. Consistency in Function Parameters:

    • In the newColumnCache function, the committed parameter is added after the Config parameter. To maintain consistency and readability, ensure that function parameters are ordered logically.
    • Recommendation: Consider reordering the parameters in the newColumnCache function for better readability and consistency.
  4. Test Coverage:

    • While the test case in column_cache_test.go has been updated to reflect the changes, it is essential to ensure comprehensive test coverage for all scenarios, including edge cases and error conditions.
    • Recommendation: Expand test coverage to validate the behavior of the code under various conditions, ensuring the correctness of the implementation.
  5. Documentation:

    • It would be beneficial to update the relevant documentation or comments to reflect the changes made in the code, especially regarding the new committed field and its implications.
    • Recommendation: Update the code comments or documentation to explain the purpose and usage of the committed field for better code understanding.

By addressing the security concern, optimizing the code structure, ensuring parameter consistency, enhancing test coverage, and updating documentation, the pull request can be improved in terms of reliability, efficiency, and maintainability.

@mergify mergify bot merged commit 4f7a721 into matrixorigin:1.1-dev Mar 22, 2024
17 of 18 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

3 participants