support restore system table (#1.2-dev)

[YANGGMM]

User description

support restore system table

Approved by: @daviszhen, @ouyuanning, @heni02

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 system table

---

PR Type

Enhancement, Tests

---

Description

• Enhanced snapshot restoration to support system tables by adding a map to specify tables to skip and implementing a function to handle system database restoration.
• Updated the logic for skipping tables during restoration to include account ID checks.
• Added conditions to skip cluster table attributes during snapshot restoration.
• Added comprehensive test cases and SQL scripts to verify the restoration of system tables, user-defined functions, stages, stored procedures, and users from snapshots.
• Included tests for restoring cluster tables from snapshots.

---

Changes walkthrough 📝

	Relevant files
Enhancement	snapshot.go Enhance snapshot restoration to support system tables.      pkg/frontend/snapshot.go Added a map needSkipTablesInMocatalog to specify which tables to skip during restore. Implemented restoreSystemDatabase function to handle restoration of system databases. Modified needSkipTable function to include account ID checks. Updated doRestoreSnapshot to check if the database needs to be skipped. +92/-15  build_show_util.go Update snapshot restoration logic for cluster table attributes. pkg/sql/plan/build_show_util.go Added conditions to skip cluster table attributes during snapshot restoration. +9/-0
Tests	snapshot_restore_system_table.result Add test cases for restoring system tables from snapshots. test/distributed/cases/snapshot/snapshot_restore_system_table.result Added test cases for restoring system tables from snapshots. Verified restoration of user-defined functions, stages, stored procedures, and users. +729/-0  snapshot_restore_system_table.sql Add SQL scripts for testing system table restoration from snapshots. test/distributed/cases/snapshot/snapshot_restore_system_table.sql Added SQL scripts to test restoring system tables from snapshots. Included tests for user-defined functions, stages, stored procedures, and users. +605/-0  snapshot_restote_cluster_table.result Add test cases for restoring cluster tables from snapshots. test/distributed/cases/snapshot/snapshot_restote_cluster_table.result Added test cases for restoring cluster tables from snapshots. +30/-0    snapshot_restote_cluster_table.sql Add SQL scripts for testing cluster table restoration from snapshots. test/distributed/cases/snapshot/snapshot_restote_cluster_table.sql Added SQL scripts to test restoring cluster tables from snapshots. +39/-0

---

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

[codiumai-pr-agent-pro]

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, due to the complexity and breadth of the changes involving system table restoration, account checks, and the handling of cluster table attributes. The PR also includes extensive test cases which need thorough review to ensure all scenarios are covered and functioning as expected.
🧪 Relevant tests	Yes
⚡ Possible issues	Possible Bug: The function needSkipTable checks if a table should be skipped based on the account ID and table name. However, the logic inside this function seems overly complex and could be prone to errors, especially with the nested conditions and different checks for system account IDs and catalog names.
	Data Integrity: The restoration process skips certain system tables and attributes based on hardcoded conditions. This might lead to incomplete data restoration if these conditions are not accurately capturing all scenarios or future system changes.
🔒 Security concerns	No

[codiumai-pr-agent-pro]

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

PR Code Suggestions ✨

Category	Suggestion	Score
Maintainability	Simplify the needSkipTablesInMocatalog map by using boolean values instead of int8 The needSkipTablesInMocatalog map can be simplified by using boolean values instead of int8. This will make the code more readable and reduce potential confusion. pkg/frontend/snapshot.go [65-98] -needSkipTablesInMocatalog = map[string]int8{ - "mo_database": 1, - "mo_tables": 1, - "mo_columns": 1, - "mo_table_partitions": 1, - "mo_foreign_keys": 1, - "mo_indexes": 1, - "mo_account": 1, - catalog.MOVersionTable: 1, - catalog.MOUpgradeTable: 1, - catalog.MOUpgradeTenantTable: 1, - catalog.MOAutoIncrTable: 1, - "mo_user": 0, - "mo_role": 0, - "mo_user_grant": 0, - "mo_role_grant": 0, - "mo_role_privs": 0, - "mo_user_defined_function": 0, - "mo_stored_procedure": 0, - "mo_mysql_compatibility_mode": 0, - "mo_stages": 0, - "mo_pubs": 1, - "mo_sessions": 1, - "mo_configurations": 1, - "mo_locks": 1, - "mo_variables": 1, - "mo_transactions": 1, - "mo_cache": 1, - "mo_snapshots": 1, +needSkipTablesInMocatalog = map[string]bool{ + "mo_database": true, + "mo_tables": true, + "mo_columns": true, + "mo_table_partitions": true, + "mo_foreign_keys": true, + "mo_indexes": true, + "mo_account": true, + catalog.MOVersionTable: true, + catalog.MOUpgradeTable: true, + catalog.MOUpgradeTenantTable: true, + catalog.MOAutoIncrTable: true, + "mo_user": false, + "mo_role": false, + "mo_user_grant": false, + "mo_role_grant": false, + "mo_role_privs": false, + "mo_user_defined_function": false, + "mo_stored_procedure": false, + "mo_mysql_compatibility_mode": false, + "mo_stages": false, + "mo_pubs": true, + "mo_sessions": true, + "mo_configurations": true, + "mo_locks": true, + "mo_variables": true, + "mo_transactions": true, + "mo_cache": true, + "mo_snapshots": true, } Suggestion importance[1-10]: 7 Why: Using boolean values instead of int8 for the needSkipTablesInMocatalog map simplifies the code and enhances readability. This is a valid improvement for maintainability.	7
Debugging	Add a log message when a table is skipped due to the needSkipTable check in restoreToDatabaseOrTable In the restoreToDatabaseOrTable function, add a log message when a table is skipped due to the needSkipTable check. This will help in debugging and understanding why certain tables are not restored. pkg/frontend/snapshot.go [550-552] for _, tblInfo := range tableInfos { key := genKey(dbName, tblInfo.tblName) + if needSkipTable(dbName, tblInfo.tblName) { + getLogger().Info(fmt.Sprintf("[%s] skip table: %v.%v", snapshotName, dbName, tblInfo.tblName)) + continue + } // skip table which is foreign key related Suggestion importance[1-10]: 6 Why: Adding a log message for skipped tables due to the needSkipTable check aids in debugging and transparency. This suggestion is correct and improves the code's debuggability.	6
	Log an error message when getTableInfos fails in the restoreSystemDatabase function In the restoreSystemDatabase function, handle the case where getTableInfos returns an error by logging the error message. This will provide more context when debugging issues related to table information retrieval. pkg/frontend/snapshot.go [577-579] tableInfos, err := getTableInfos(ctx, bh, snapshotName, moCatalog, "") if err != nil { + getLogger().Error(fmt.Sprintf("Failed to get table infos for %s: %v", moCatalog, err)) return } Suggestion importance[1-10]: 6 Why: Logging an error when getTableInfos fails provides more context and aids in debugging. This is a correct and useful suggestion for error handling.	6
Readability	Combine the if conditions in ConstructCreateTableSQL to improve readability In the ConstructCreateTableSQL function, combine the two if conditions that check util.IsClusterTableAttribute and isClusterTable to improve readability and reduce nesting. pkg/sql/plan/build_show_util.go [73-77] -if util.IsClusterTableAttribute(colName) && - isClusterTable && - accountId == catalog.System_Account && - !snapshot.TS.Equal(timestamp.Timestamp{}) { +if util.IsClusterTableAttribute(colName) && isClusterTable && + accountId == catalog.System_Account && !snapshot.TS.Equal(timestamp.Timestamp{}) { continue } Suggestion importance[1-10]: 5 Why: Combining the conditions into a single line reduces nesting and improves readability, although it's a minor stylistic change. This suggestion is correct and enhances code clarity.	5