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

skip collecting mo_tables, mo_databases, mo_columns and cluster tables for non-sys. #16512

Merged

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

Enhancement, Bug fix


Description

  • Added specialTableFilterForNonSys function to filter out mo_tables, mo_databases, mo_columns, and cluster tables for non-sys accounts.
  • Removed unnecessary account switching logic for cluster tables in MoTableRows and MoTableSize functions.
  • Replaced context ctx with foolCtx for database operations to ensure proper context handling.
  • Simplified logic for handling partitioned tables in MoTableRows.
  • Commented out the gathering of special table sizes in HandleStorageUsage to prevent incorrect size calculations.
  • Disabled handling of special sizes for new accounts that are not ready in HandleStorageUsage.

Changes walkthrough 📝

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

pkg/sql/plan/function/func_mo.go

  • Added specialTableFilterForNonSys function to filter special tables
    for non-sys accounts.
  • Removed account switching logic for cluster tables.
  • Replaced context ctx with foolCtx for database operations.
  • Simplified logic for handling partitioned tables.
  • +45/-30 
    Bug fix
    handle_debug.go
    Disable special table size handling in storage usage         

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

  • Commented out the gathering of special table sizes.
  • Disabled handling of special sizes for new accounts not ready.
  • +7/-6     

    💡 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 across multiple functions with modifications in logic and context handling, which requires a careful review to ensure that the new logic correctly implements the intended behavior without introducing regressions.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The specialTableFilterForNonSys function returns immediately if the account ID is the system account or the database string is not catalog.MO_CATALOG, which might skip necessary checks for cluster tables in system accounts.

    Context Handling: The replacement of proc.Ctx with foolCtx in multiple places needs thorough testing to ensure that the context is correctly managed and does not lead to unintended side effects or security issues.

    🔒 Security concerns

    No

    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
    Enhancement
    Use a raw string literal with regexp.MustCompile for better readability and to avoid unnecessary string formatting

    Instead of using fmt.Sprintf to create the regular expression pattern, consider using
    regexp.MustCompile directly with a raw string literal. This will make the code more
    readable and avoid unnecessary string formatting.

    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(`catalog.MO_TABLES|catalog.MO_DATABASE|catalog.MO_COLUMNS`)
     
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly identifies an opportunity to simplify the regular expression initialization using a raw string literal, which enhances readability and efficiency.

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

    Remove the commented-out code to keep the codebase clean and maintainable. If the code is
    needed for future reference, consider adding a comment explaining why it is commented out.

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

    -//specialSize := memo.GatherSpecialTableSize()
    -//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")
    -//}
     
    +
    Suggestion importance[1-10]: 6

    Why: Removing commented-out code can indeed help in maintaining a cleaner codebase. The suggestion is valid and promotes good coding practices.

    6

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label May 30, 2024
    @gouhongshen gouhongshen changed the title skip collectingmo_tables, mo_databases, mo_columns and cluster tables for non-sys. skip collecting mo_tables, mo_databases, mo_columns and cluster tables for non-sys. May 30, 2024
    @mergify mergify bot merged commit 0081545 into matrixorigin:main May 30, 2024
    16 of 18 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants