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

sync to 1.2: skip collecting mo_tables, mo_databases, mo_columns and cluster tables for non-sys. #16515

Conversation

gouhongshen
Copy link
Contributor

@gouhongshen gouhongshen commented May 30, 2024

User description

What type of PR is this?

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

Which issue(s) this PR fixes:

issue ##16419

What this PR does / why we need it:

skip collecting mo_tables, mo_databases, mo_columns and cluster tables for non-sys when mo_table_size, mo_table_rows and show accounts.


PR Type

Bug fix, Enhancement


Description

  • Added a new function specialTableFilterForNonSys to filter special tables (mo_tables, mo_databases, mo_columns) and cluster tables for non-sys accounts.
  • Removed the old logic that switched accounts for cluster tables, simplifying the context handling.
  • Updated the MoTableRows and MoTableSize functions to use the new filtering logic and consistent context variable foolCtx.
  • Disabled the gathering of special table sizes and handling of not ready new accounts in the HandleStorageUsage function.

Changes walkthrough 📝

Relevant files
Enhancement
func_mo.go
Filter special tables for non-sys accounts and refactor context
handling

pkg/sql/plan/function/func_mo.go

  • Added specialTableFilterForNonSys function to filter special tables
    for non-sys accounts.
  • Removed old account switching logic for cluster tables.
  • Replaced context variable ctx with foolCtx for consistency.
  • +45/-30 
    Bug fix
    handle_debug.go
    Disable special table size gathering and not ready new account
    handling

    pkg/vm/engine/tae/rpc/handle_debug.go

  • Commented out the usage of GatherSpecialTableSize.
  • Commented out handling for not ready new accounts.
  • +11/-9   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves changes to core functions with logic that filters data based on user permissions and table metadata. Understanding the context and ensuring no regressions occur would require a moderate level of effort.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The new function specialTableFilterForNonSys might not correctly handle all edge cases related to account IDs and table names due to its reliance on regular expressions and string matching. This could lead to incorrect filtering under certain conditions.

    Performance Concern: The changes in context handling (foolCtx) might impact performance if not properly managed, especially in loops querying database relations.

    🔒 Security concerns

    No

    @mergify mergify bot requested a review from sukki37 May 30, 2024 08:09
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Remove commented-out code to keep the codebase clean and maintainable

    The commented-out code can be removed if it is no longer needed to keep the codebase clean
    and maintainable.

    pkg/vm/engine/tae/rpc/handle_debug.go [87-138]

    -//specialSize := memo.GatherSpecialTableSize()
    -...
    -//var notReadyNewAcc []uint64
    -...
    -//else {
    -//	notReadyNewAcc = append(notReadyNewAcc, newIds[idx])
    -//}
    -...
    -//for idx := range notReadyNewAcc {
    -//	resp.AccIds = append(resp.AccIds, int64(notReadyNewAcc[idx]))
    -//	resp.Sizes = append(resp.Sizes, specialSize)
    -//	memo.AddReqTrace(uint64(newIds[idx]), specialSize, start, "new, not ready, only special")
    -//}
    +// Remove the commented-out code
     
    Suggestion importance[1-10]: 7

    Why: Removing commented-out code is a good practice for maintaining a clean codebase. The suggestion correctly identifies multiple instances of commented-out code that should be cleaned up.

    7
    Consolidate error handling to improve readability and reduce redundancy

    Instead of checking err multiple times and returning it, consider consolidating the error
    handling to improve readability and reduce redundancy.

    pkg/sql/plan/function/func_mo.go [84-118]

    +if err != nil {
    +    return err
    +}
    +...
    +if err != nil {
    +    return err
    +}
    +...
    +if err != nil {
    +    return err
    +}
     
    -
    Suggestion importance[1-10]: 6

    Why: The suggestion correctly identifies the repetitive error handling pattern and proposes a consolidation. However, the exact implementation of this consolidation is not detailed, making the suggestion somewhat incomplete.

    6
    Enhancement
    Simplify the regular expression definition by using a raw string literal

    The specialRegexp variable is constructed using fmt.Sprintf which might be unnecessary.
    Consider using a raw string literal to define the regular expression directly.

    pkg/sql/plan/function/func_mo.go [249-251]

    -var specialRegexp = regexp.MustCompile(fmt.Sprintf("%s|%s|%s",
    -    catalog.MO_TABLES, catalog.MO_DATABASE, catalog.MO_COLUMNS))
    +var specialRegexp = regexp.MustCompile(`MO_TABLES|MO_DATABASE|MO_COLUMNS`)
     
    Suggestion importance[1-10]: 5

    Why: The suggestion to simplify the regular expression definition is valid and would enhance readability. However, the use of fmt.Sprintf might be intentional for dynamic construction, so the suggestion might not be applicable in all contexts.

    5

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label May 30, 2024
    @mergify mergify bot merged commit 6b9f5de into matrixorigin:1.2-dev May 31, 2024
    16 of 18 checks passed
    @aylei aylei mentioned this pull request Jun 5, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix Enhancement Review effort [1-5]: 3 size/S Denotes a PR that changes [10,99] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    5 participants