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

support restore publication db (#1.2-dev) #16488

Merged
merged 9 commits into from
Jun 4, 2024

Conversation

YANGGMM
Copy link
Contributor

@YANGGMM YANGGMM commented May 30, 2024

User description

support restore publication db


PR Type

Feature, Tests


Description

  • Added support for deleting publication from the database.
  • Added shared transaction background execution function.
  • Added functions to handle publication database operations during snapshot restoration.
  • Added test cases and SQL scripts for restoring publication database from snapshot.

Changes walkthrough 📝

Relevant files
Enhancement
authenticate.go
Add support for deleting publication from database and shared
transaction execution.

pkg/frontend/authenticate.go

  • Added SQL format for deleting publication from database.
  • Added function to get SQL for deleting publication from database.
  • Modified isDbPublishing to use shared transaction background
    execution.
  • +11/-8   
    back_exec.go
    Add shared transaction background execution function.       

    pkg/frontend/back_exec.go

    • Added function to get shared transaction background execution.
    +40/-0   
    snapshot.go
    Add support for publication database operations in snapshot
    restoration.

    pkg/frontend/snapshot.go

  • Added constants and SQL formats for publication database operations.
  • Added functions to get SQL for publication count with snapshot and
    restore publication record.
  • Added functions to check and drop publication record, and to check and
    restore publication record.
  • +129/-0 
    types.go
    Add shared transaction background execution method to FeSession
    interface.

    pkg/frontend/types.go

    • Added GetShareTxnBackgroundExec method to FeSession interface.
    +1/-0     
    Tests
    snapshot_restore_publication.result
    Add test cases for restoring publication database from snapshot.

    test/distributed/cases/snapshot/snapshot_restore_publication.result

    • Added test cases for restoring publication database from snapshot.
    +446/-0 
    snapshot_restore_publication.sql
    Add SQL scripts for testing publication database restoration from
    snapshot.

    test/distributed/cases/snapshot/snapshot_restore_publication.sql

  • Added SQL scripts for testing publication database restoration from
    snapshot.
  • +402/-0 

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

    Approved by: @daviszhen, @aressu1985

    What type of PR is this?

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

    Which issue(s) this PR fixes:

    issue #16353

    What this PR does / why we need it:

    support restore publication db


    PR Type

    Enhancement, Tests


    Description

    • Added support for deleting publication from the database.
    • Added shared transaction background execution function.
    • Added functions to handle publication database operations during snapshot restoration.
    • Added test cases and SQL scripts for restoring publication database from snapshot.

    Changes walkthrough 📝

    Relevant files
    Enhancement
    authenticate.go
    Add support for deleting publication and shared transaction execution

    pkg/frontend/authenticate.go

  • Added SQL format for deleting publication from the database.
  • Added function to get SQL for deleting publication from the database.
  • Modified isDbPublishing to use shared transaction background
    execution.
  • +11/-8   
    back_exec.go
    Add shared transaction background execution function         

    pkg/frontend/back_exec.go

  • Added function GetShareTxnBackgroundExec for shared transaction
    background execution.
  • +39/-0   
    snapshot.go
    Add functions for publication database operations during snapshot
    restoration

    pkg/frontend/snapshot.go

  • Added SQL formats and functions for handling publication database
    operations during snapshot restoration.
  • Added functions checkPubAndDropPubRecord and
    checkAndRestorePublicationRecord for publication record management.
  • +129/-0 
    types.go
    Add shared transaction execution method to FeSession interface

    pkg/frontend/types.go

    • Added GetShareTxnBackgroundExec method to FeSession interface.
    +1/-0     
    Tests
    snapshot_restore_publication.result
    Add test cases for publication database restoration           

    test/distributed/cases/snapshot/snapshot_restore_publication.result

    • Added test cases for restoring publication database from snapshot.
    +442/-0 
    snapshot_restore_publication.sql
    Add SQL scripts for publication database restoration tests

    test/distributed/cases/snapshot/snapshot_restore_publication.sql

  • Added SQL scripts for testing publication database restoration from
    snapshot.
  • +402/-0 

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

    support restore publication db
    
    
    ___
    
    ### **PR Type**
    Feature, Tests
    
    
    ___
    
    ### **Description**
    - Added support for deleting publication from the database.
    - Added shared transaction background execution function.
    - Added functions to handle publication database operations during snapshot restoration.
    - Added test cases and SQL scripts for restoring publication database from snapshot.
    
    
    
    ___
    
    
    
    ### **Changes walkthrough** 📝
    <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement
    </strong></td><td><table>
    <tr>
    <td>
    <details>
    <summary><strong>authenticate.go</strong><dd><code>Add support for deleting publication from database and shared </code><br><code>transaction execution.</code></dd></summary>
    <hr>
    
    pkg/frontend/authenticate.go
    
    <li>Added SQL format for deleting publication from database.<br> <li> Added function to get SQL for deleting publication from database.<br> <li> Modified <code>isDbPublishing</code> to use shared transaction background <br>execution.<br>
    
    
    </details>
    
    
    </td>
    <td><a href="https://github.com/matrixorigin/matrixone/pull/16448/files#diff-849f201c351210bd95807e99d1538e2602a5244b256c35e58467afe304c509e6">+11/-8</a>&nbsp; &nbsp; </td>
    
    </tr>
    
    <tr>
    <td>
    <details>
    <summary><strong>back_exec.go</strong><dd><code>Add shared transaction background execution function.</code>&nbsp; &nbsp; &nbsp; &nbsp; </dd></summary>
    <hr>
    
    pkg/frontend/back_exec.go
    
    - Added function to get shared transaction background execution.
    
    
    
    </details>
    
    
    </td>
    <td><a href="https://github.com/matrixorigin/matrixone/pull/16448/files#diff-6c8c7605ddaba02dbfd483c021e3901f66c8205e467634fde29e0dbdbb79396a">+40/-0</a>&nbsp; &nbsp; </td>
    
    </tr>
    
    <tr>
    <td>
    <details>
    <summary><strong>snapshot.go</strong><dd><code>Add support for publication database operations in snapshot </code><br><code>restoration.</code></dd></summary>
    <hr>
    
    pkg/frontend/snapshot.go
    
    <li>Added constants and SQL formats for publication database operations.<br> <li> Added functions to get SQL for publication count with snapshot and <br>restore publication record.<br> <li> Added functions to check and drop publication record, and to check and <br>restore publication record.<br>
    
    
    </details>
    
    
    </td>
    <td><a href="https://github.com/matrixorigin/matrixone/pull/16448/files#diff-2774e67c5debc02b9642113381977ab7c4c591114aabe013627c63851d0c2f9c">+129/-0</a>&nbsp; </td>
    
    </tr>
    
    <tr>
    <td>
    <details>
    <summary><strong>types.go</strong><dd><code>Add shared transaction background execution method to FeSession </code><br><code>interface.</code></dd></summary>
    <hr>
    
    pkg/frontend/types.go
    
    - Added `GetShareTxnBackgroundExec` method to `FeSession` interface.
    
    
    
    </details>
    
    
    </td>
    <td><a href="https://github.com/matrixorigin/matrixone/pull/16448/files#diff-5426ed8bd66760011299bcef0a9d1d552477eaae49a8451e16b8ec5bf7d7310f">+1/-0</a>&nbsp; &nbsp; &nbsp; </td>
    
    </tr>
    </table></td></tr><tr><td><strong>Tests
    </strong></td><td><table>
    <tr>
    <td>
    <details>
    <summary><strong>snapshot_restore_publication.result</strong><dd><code>Add test cases for restoring publication database from snapshot.</code></dd></summary>
    <hr>
    
    test/distributed/cases/snapshot/snapshot_restore_publication.result
    
    - Added test cases for restoring publication database from snapshot.
    
    
    
    </details>
    
    
    </td>
    <td><a href="https://github.com/matrixorigin/matrixone/pull/16448/files#diff-cd27b7e6665f58d3df027239321cc738648d682e32282cdba3bc4623a1f6339e">+446/-0</a>&nbsp; </td>
    
    </tr>
    
    <tr>
    <td>
    <details>
    <summary><strong>snapshot_restore_publication.sql</strong><dd><code>Add SQL scripts for testing publication database restoration from </code><br><code>snapshot.</code></dd></summary>
    <hr>
    
    test/distributed/cases/snapshot/snapshot_restore_publication.sql
    
    <li>Added SQL scripts for testing publication database restoration from <br>snapshot.<br>
    
    
    </details>
    
    
    </td>
    <td><a href="https://github.com/matrixorigin/matrixone/pull/16448/files#diff-628fac0767093e07f75ae64707642fb96eb836b169c4d011013cfda409934fa3">+402/-0</a>&nbsp; </td>
    
    </tr>
    </table></td></tr></tr></tbody></table>
    
    ___
    
    > 💡 **PR-Agent usage**:
    >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions
    
    Approved by: @daviszhen, @aressu1985
    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]

    4, because the PR introduces multiple complex changes across several files, including SQL operations, transaction management, and snapshot restoration logic. The changes are spread across authentication, execution, and snapshot handling, which requires a thorough understanding of the existing system architecture and data flow.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The function getSqlForDeletePubFromDatabase in authenticate.go does not handle potential SQL injection risks due to direct string formatting. Consider using parameterized queries or proper sanitization.

    Performance Concern: The function isDbPublishing in authenticate.go has been modified to use a shared transaction background execution. This change could potentially impact the performance if not properly managed, especially under high load.

    🔒 Security concerns

    SQL Injection: The use of string formatting in SQL query construction in several functions such as getSqlForDeletePubFromDatabase and getSqlForGetDbPubCountWithSnapshot could lead to SQL injection vulnerabilities if the input is not properly sanitized.

    @matrix-meow matrix-meow added the size/XL Denotes a PR that changes [1000, 1999] lines label May 30, 2024
    @mergify mergify bot requested a review from sukki37 May 30, 2024 01:54
    @mergify mergify bot added the kind/feature label May 30, 2024
    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
    Security
    Use parameterized queries to prevent SQL injection attacks

    The restorePubDbDataFmt format string should use parameterized queries to prevent SQL
    injection attacks. Directly formatting SQL strings with user input can be dangerous.

    pkg/frontend/snapshot.go [68]

    -restorePubDbDataFmt = "insert into `%s`.`%s` SELECT * FROM `%s`.`%s` {snapshot = '%s'} WHERE  DATABASE_NAME = '%s'"
    +restorePubDbDataFmt = "insert into ?.? SELECT * FROM ?.? {snapshot = ?} WHERE DATABASE_NAME = ?"
     
    Suggestion importance[1-10]: 10

    Why: This suggestion correctly identifies a critical security issue related to SQL injection, which is a major concern in software development. The suggestion to use parameterized queries is appropriate and necessary to enhance the security of the code.

    10
    Best practice
    Use drop account if exists to avoid errors if the accounts do not exist

    To ensure that the drop account statements do not fail if the accounts do not exist,
    consider using drop account if exists instead of just drop account.

    test/distributed/cases/snapshot/snapshot_restore_publication.sql [53-54]

    -drop account test_tenant_1;
    -drop account test_tenant_2;
    +drop account if exists test_tenant_1;
    +drop account if exists test_tenant_2;
     
    Suggestion importance[1-10]: 10

    Why: The suggestion correctly identifies the risk of failing drop account statements and recommends a safer alternative. This is crucial to avoid runtime errors in SQL scripts.

    10
    Use drop publication if exists to avoid errors if the publications do not exist

    To ensure that the drop publication statements do not fail if the publications do not
    exist, consider using drop publication if exists instead of just drop publication.

    test/distributed/cases/snapshot/snapshot_restore_publication.sql [116-117]

    -drop publication pubname1;
    -drop publication pubname2;
    +drop publication if exists pubname1;
    +drop publication if exists pubname2;
     
    Suggestion importance[1-10]: 10

    Why: This suggestion is accurate and addresses a potential error by recommending the use of drop publication if exists. It enhances the robustness of the SQL script.

    10
    Use drop database if exists to avoid errors if the databases do not exist

    To ensure that the drop database statements do not fail if the databases do not exist,
    consider using drop database if exists instead of just drop database.

    test/distributed/cases/snapshot/snapshot_restore_publication.sql [118-119]

    -drop database db1;
    -drop database db2;
    +drop database if exists db1;
    +drop database if exists db2;
     
    Suggestion importance[1-10]: 10

    Why: The suggestion is valid and improves the script by preventing errors from drop database commands when the databases might not exist. This is a best practice for SQL database management.

    10
    Restore the original context after modifying it to maintain consistency

    In the checkAndRestorePublicationRecord function, the context should be restored to its
    original state after modifying it with defines.AttachAccountId. This ensures that the
    context remains consistent for subsequent operations.

    pkg/frontend/snapshot.go [1192]

    +originalCtx := ctx
     ctx = defines.AttachAccountId(ctx, toAccountId)
    +defer func() { ctx = originalCtx }()
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves the maintainability and reliability of the code by ensuring that the context's original state is preserved. This is a good practice in handling context modifications, especially in a multi-threaded environment.

    7
    Ensure errors from inputNameIsInvalid are properly handled

    The getSqlForDeletePubFromDatabase function should return an error if inputNameIsInvalid
    returns an error. This ensures that invalid database names are properly handled.

    pkg/frontend/authenticate.go [1893-1897]

    -err := inputNameIsInvalid(ctx, dbName)
    -if err != nil {
    +if err := inputNameIsInvalid(ctx, dbName); err != nil {
         return "", err
     }
     return fmt.Sprintf(deletePubFromDatabaseFormat, dbName), nil
     
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly emphasizes the importance of error handling, ensuring that the function behaves as expected when encountering invalid input. This is a crucial aspect of robust error management in software applications.

    7
    Maintainability
    Move bh.ClearExecResultSet() to immediately after bh.Exec to ensure it is called only when necessary

    In the checkPubAndDropPubRecord function, the bh.ClearExecResultSet() call should be moved
    to immediately after the bh.Exec call to ensure the result set is cleared only when
    necessary.

    pkg/frontend/snapshot.go [1123-1125]

    -bh.ClearExecResultSet()
     if err = bh.Exec(ctx, sql); err != nil {
         return
     }
    +bh.ClearExecResultSet()
     
    Suggestion importance[1-10]: 6

    Why: This suggestion enhances the maintainability of the code by ensuring that resources are managed more efficiently. Clearing the result set immediately after execution prevents potential issues related to stale data, although it's a relatively minor improvement in the context of the entire function.

    6

    @YANGGMM
    Copy link
    Contributor Author

    YANGGMM commented May 30, 2024

    需要等修改 pub的pr合进去

    @matrix-meow matrix-meow added size/L Denotes a PR that changes [500,999] lines and removed size/XL Denotes a PR that changes [1000, 1999] lines labels Jun 3, 2024
    @sukki37 sukki37 merged commit 1ef0439 into matrixorigin:1.2-dev Jun 4, 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
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    5 participants