[Feature] Support new syntax for Backup/Restore #52729

srlch · 2024-11-08T03:24:32Z

Why we need new syntax for Backup/Restore

The semantic design of the old syntax is not easy to understand and is not user-friendly.
The newly introduced syntax has better scalability and flexibility to support (multiple) catalog, multiple object type and multiple DBs Backup/Restore in the future.

New syntax design for Backup/Restore

BACKUP [DATABASE <db_name>] SNAPSHOT [<dbname>.]<snapshot_name>
TO <repo_name>
[ ON ( backup_restore_object [, ...] ) ]
[PROPERTIES]

RESTORE SNAPSHOT [<dbname>.]<snapshot_name> FROM <repo_name>
[DATABASE <db_name> [AS <db_alias>] ]
[ ON ( backup_restore_object [, ...] ) ]
[PROPERTIES]

backup_restore_object ::=
    ALL TABLE[S]             | (TABLE | TABLES) <table_name> [ PARTITION (...) ] [AS <alias>] |
    ALL MATERIALIZED VIEW[S] | MATERIALIZED (VIEW | VIEWS) <mv_name> [AS <alias>] |
    ALL VIEW[S]              | (VIEW | VIEWS) <view_name> [AS <alias>] |
    ALL FUNCTION[S]          | (FUNCTION | FUNCTIONS) <func_name> [AS <alias>] |
    <table_name> [ PARTITION (...) ] [AS <alias>]

The behavioral changes in Backup/Restore syntax

Expansion of ON clause:
We introduce the key word (TABLE(S)/VIEW(S)/MATERIALIZED VIEW(S)/FUNCTION(S)) to identify different type of Backup/Restore object and use ALL to represent all objects of a certain type which is much more clear the before.

Allow to specify database explicitly and separated from snapshot name:
Backup: User can specify database after DATABASE key word or before snapshot name as before.

Restore:

If user does not specify dbname at all (both before snapshot name or after DATABASE keyword) without ON clause, it will create database with the same name in snapshot file and restore all data into it.
If specify dbName after DATABASE keyword, no dbName can be specified before snapshot name.
The dbName after DATABASE keyword must exactly match in snapshot file. DATABASE <db_name> [AS <db_alias>] means that restore the database named by db_name in snapshot to the database named by db_alias in current cluster. If AS is missing, restore to the database with the same name.

Fixes #52746

What type of PR is this:

Does this PR entail a change in behavior?

Yes, this PR will result in a change in behavior.
No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

Interface/UI changes: syntax, type conversion, expression evaluation, display information
Parameter changes: default values, similar parameters but with different default values
Policy changes: use new policy to replace old one, functionality automatically enabled
Feature removed
Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

I have added test cases for my bug fix or my new feature
This pr needs user documentation (for new or modified features or behaviors)
- I have added documentation for my new feature or new function
This is a backport pr

Bugfix cherry-pick branch check:

starrocks-cr · 2024-11-08T03:26:20Z

fe/fe-core/src/main/java/com/starrocks/sql/parser/AstBuilder.java

@@ -8246,6 +8356,7 @@ public List<String> getColumnNames(StarRocksParser.ColumnAliasesContext context)
    }

    protected NodePosition createPos(ParserRuleContext context) {
+        Preconditions.checkState(context != null);
        return createPos(context.start, context.stop);
    }



Who sings the song "Runnin' Down a Dream"?

starrocks-cr · 2024-11-08T03:26:23Z

fe/fe-core/src/main/java/com/starrocks/sql/analyzer/BackupRestoreAnalyzer.java

@@ -136,7 +151,7 @@ public Void visitBackupStatement(BackupStmt backupStmt, ConnectContext context)

            // analyze and get Function for stmt
            List<FunctionRef> fnRefs = backupStmt.getFnRefs();
-            if (!withOnClause) {
+            if (!withOnClause || allFunction) /* without `On` or contains `ALL` */ {
                fnRefs.add(new FunctionRef(database.getFunctions()));
            } else {
                backupStmt.getFnRefs().stream().forEach(x -> x.analyzeForBackup(database));


The most risky bug in this code is:
Potential unintended backup inclusion due to condition logic.

You can modify the code like this:

// Adjust the conditional logic for backing up all tables, MVs, or views correctly if (!withOnClause && (allTable || allMV || allView)) { for (Table tbl : GlobalStateMgr.getCurrentState().getLocalMetastore().getTables(database.getId())) { if (!Config.enable_backup_materialized_view && tbl.isMaterializedView()) { LOG.info("Skip backup materialized view: {} because " + "backup of materialized views is disabled", tbl.getName()); continue; } if (tbl.isTemporaryTable()) { continue; } if ((tbl.isOlapTable() && !allTable) || (tbl.isOlapMaterializedView() && !allMV) || (tbl.isOlapView() && !allView)) { continue; } TableName tableName = new TableName(dbName, tbl.getName()); TableRef tableRef = new TableRef(tableName, null, null); tableRefs.add(tableRef); } }

This modification clarifies that all specific objects should only be added when the corresponding flags are set, correcting logical errors from the original inclusive checks.

starrocks-cr · 2024-11-08T03:26:26Z

fe/fe-core/src/main/java/com/starrocks/backup/BackupHandler.java

+                               .filter(x -> x.isOlapView()).map(x -> x.getName()).collect(Collectors.toSet()));
+            }
+        }
+
        // only retain restore tables
        jobInfo.retainTables(allTbls);
    }


The most risky bug in this code is:
A null pointer dereference when dbName is null during the creation or access of the database.

You can modify the code like this:

// check if db exist String dbName = stmt.getDbName(); Database db = globalStateMgr.getLocalMetastore().getDb(dbName); if (db == null) { if (stmt instanceof RestoreStmt) { // Ensure jobInfo is initialized before using it if (jobInfo == null && dbName != null) { ErrorReport.reportDdlException(ErrorCode.ERR_COMMON_ERROR, "JobInfo is not available for database: " + dbName); } if (dbName == null) { // Check if jobInfo has the necessary data to avoid null pointer exception if (jobInfo == null || jobInfo.dbName == null) { ErrorReport.reportDdlException(ErrorCode.ERR_COMMON_ERROR, "Cannot determine database name from job info during restore process"); } // use dbName in snapshot if target dbName is null dbName = jobInfo.dbName; } try { globalStateMgr.getLocalMetastore().createDb(dbName, null); db = globalStateMgr.getLocalMetastore().getDb(dbName); } catch (Exception e) { ErrorReport.reportDdlException(ErrorCode.ERR_COMMON_ERROR, "Cannot create database: " + dbName + " in restore process"); } } else { ErrorReport.reportDdlException(ErrorCode.ERR_BAD_DB_ERROR, dbName); } }

Signed-off-by: srlch <[email protected]>

github-actions · 2024-11-11T09:11:21Z

[FE Incremental Coverage Report]

✅ pass : 60 / 69 (86.96%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	com/starrocks/backup/BackupHandler.java	29	37	78.38%	[274, 275, 276, 298, 301, 303, 305, 473]
🔵	com/starrocks/sql/analyzer/BackupRestoreAnalyzer.java	8	9	88.89%	[109]
🔵	com/starrocks/sql/ast/AbstractBackupStmt.java	23	23	100.00%	[]

jaogoy · 2024-11-11T08:32:58Z

fe/fe-core/src/main/java/com/starrocks/sql/parser/StarRocks.g4

-    : tableDesc | FUNCTION qualifiedName
+backupRestoreObjectDesc
+    : backupRestoreTableDesc
+    | (ALL (FUNCTION | FUNCTIONS) | (FUNCTION | FUNCTIONS) qualifiedName (AS identifier)?)


Why is ( ) needed here? This syntax can't restrict that there is only one ALL FUNCTIONS or multiple specified FUNCTIONS.

And, maybe I want to restore all Functions, and restore some of them with alias. Can it be supported.

() is no needed here, () is just a marker for the syntax definition in .g4 file. ALL FUNCTIONS and FUNCTIONS can be specified multiple time, check definition in backupStatement

If use ALL FUNCTIONS, alias can not be set in this case

jaogoy · 2024-11-11T08:39:47Z

fe/fe-core/src/main/java/com/starrocks/sql/common/ParserErrorMsg.java

+    @BaseMessage("Specify alias for backup object is forbidden in BACKUP stmt")
+    String unsupportedSepcifyAliasInBackupStmt();
+
+    @BaseMessage("`ON` clause is forbidden if no Database explicitly completely in Restore stmt")


what's the meaning of explicity completely?

jaogoy · 2024-11-11T08:40:14Z

fe/fe-core/src/main/java/com/starrocks/sql/common/ParserErrorMsg.java

@@ -222,4 +222,13 @@ public interface ParserErrorMsg {
    String nullIdentifierCancelBackupRestore();
    @BaseMessage("Value count in PIVOT {0} must match number of FOR columns {1}")
    String pivotValueArityMismatch(int a0, int a1);
+
+    @BaseMessage("Specify dbName after snapshot name is forbidden if the DbName is specified explicitly in BACKUP/RESTORE")


after -> before?

jaogoy · 2024-11-11T08:43:52Z

fe/fe-core/src/main/java/com/starrocks/sql/ast/AbstractBackupStmt.java

-        this.withOnClause = !(this.tblRefs.isEmpty() && this.fnRefs.isEmpty());
+        this.originDbName = "";
+
+        this.withOnClause = false;


Why not directly set the default in variable declaration?

jaogoy · 2024-11-11T08:46:56Z

fe/fe-core/src/main/java/com/starrocks/sql/parser/AstBuilder.java

-    public ParseNode visitBackupStatement(StarRocksParser.BackupStatementContext context) {
-        QualifiedName qualifiedName = getQualifiedName(context.qualifiedName());
-        LabelName labelName = qualifiedNameToLabelName(qualifiedName);
+    private ParseNode getObjectRef(StarRocksParser.QualifiedNameContext qualifiedNameContext,


It'll be clearer to split it into 2 functions: getFunctionRef, getTableRef

jaogoy · 2024-11-11T08:55:32Z

fe/fe-core/src/main/java/com/starrocks/sql/parser/StarRocks.g4

-    FROM identifier
-    (ON '(' restoreObjectDesc (',' restoreObjectDesc) * ')')?
+    FROM repoName=identifier
+    (DATABASE dbName=identifier (AS dbAlias=identifier)?)?


Will it be simpler to extract a sub-statement like:

backupRestorePackageStmt DATABASE dbName=identifier (AS dbAlias=identifier)? | .....

Other pageage types will be supported/used later.

I will consider this change in next pr for external catalog. It seems that backupRestorePackageStmt designed for DATABASE and CATALOG is better.

jaogoy · 2024-11-11T09:09:38Z

fe/fe-core/src/main/java/com/starrocks/sql/parser/AstBuilder.java

-        return new BackupStmt(labelName, repoName, tblRefs, fnRefs, properties, createPos(context));
+        AbstractBackupStmt stmt = null;
+        if (backupContext != null) {
+            stmt = (AbstractBackupStmt) (new BackupStmt(labelName, repoName, mixTblRefs,


(AbstractBackupStmt) is not needed.

jaogoy · 2024-11-11T09:15:19Z

fe/fe-core/src/main/java/com/starrocks/sql/parser/AstBuilder.java

+        String originDb = null;
+
+        boolean withOnClause = false;
+        boolean allTable = false;


I think, it will be simpler to use vector to store allXxx and Refs, like:

ArrayList allObjects = false * 4. with enum TABLE/MV/VIEW/FUNCTION.

ArrayList<ArrayList<>> allRefs = {mixTblRefs, mixTblRefs, mixTblRefs, new List()};

Fixed NO.1, for NO.2 it seems that keep the original implementation is better, because we will treat TABLE/MV/VIEW as TableRef.

Signed-off-by: srlch <[email protected]>

jaogoy · 2024-11-13T03:29:42Z

fe/fe-core/src/main/java/com/starrocks/sql/ast/AbstractBackupStmt.java


 public class AbstractBackupStmt extends DdlStmt {
+    public enum BackupRestoreAllMarker {


BackupObjectType

jaogoy · 2024-11-13T03:33:30Z

fe/fe-core/src/main/java/com/starrocks/sql/common/ParserErrorMsg.java

@@ -223,12 +223,12 @@ public interface ParserErrorMsg {
    @BaseMessage("Value count in PIVOT {0} must match number of FOR columns {1}")
    String pivotValueArityMismatch(int a0, int a1);

-    @BaseMessage("Specify dbName after snapshot name is forbidden if the DbName is specified explicitly in BACKUP/RESTORE")
+    @BaseMessage("Specify dbName before snapshot name is forbidden if the DbName is specified explicitly in BACKUP/RESTORE")


Specifying ...

jaogoy · 2024-11-13T03:34:55Z

fe/fe-core/src/main/java/com/starrocks/sql/parser/AstBuilder.java

        }
+        TableRef tableRef = new TableRef(tableName, alias, partitionNames, position);


Why not directly return new TableRef(...);

jaogoy · 2024-11-13T03:36:29Z

fe/fe-core/src/main/java/com/starrocks/sql/parser/AstBuilder.java

@@ -3424,58 +3423,58 @@ private ParseNode parseBackupRestoreStatement(ParserRuleContext context) {
            }

            if (specifiedFunction) {
-                if (allFunction) {
+                if (allMarker.contains(AbstractBackupStmt.BackupRestoreAllMarker.FUNCTION)) {


directly import xxx.AbstractBackupStmt.BackupObjectType;
Then usage is simpler.

github-actions · 2024-11-13T03:51:50Z

[Java-Extensions Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

github-actions · 2024-11-13T03:53:35Z

[BE Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

Signed-off-by: srlch <[email protected]>

sonarcloud · 2024-11-13T07:48:37Z

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Signed-off-by: srlch <[email protected]>

srlch requested review from a team as code owners November 8, 2024 03:24

github-actions bot added the title needs [type] label Nov 8, 2024

mergify bot assigned srlch Nov 8, 2024

starrocks-cr bot reviewed Nov 8, 2024

View reviewed changes

[Feature] support new syntax for backup/restore

98144e8

Signed-off-by: srlch <[email protected]>

srlch force-pushed the support_new_syntax branch from d0dc1d4 to 98144e8 Compare November 8, 2024 08:38

srlch changed the title ~~[WIP] support new syntax for backup restore~~ [Feature] support new syntax for backup restore Nov 8, 2024

github-actions bot added behavior_changed and removed title needs [type] labels Nov 8, 2024

srlch changed the title ~~[Feature] support new syntax for backup restore~~ [Feature] Support new syntax for Backup/Restore Nov 8, 2024

ADD more ut

9c1946d

Signed-off-by: srlch <[email protected]>

jaogoy reviewed Nov 11, 2024

View reviewed changes

fix comment

90b01e5

Signed-off-by: srlch <[email protected]>

srlch force-pushed the support_new_syntax branch from b465aa8 to 90b01e5 Compare November 13, 2024 02:27

refactor

43d9a2f

Signed-off-by: srlch <[email protected]>

jaogoy reviewed Nov 13, 2024

View reviewed changes

srlch added 2 commits November 13, 2024 13:42

fix comment

7025b84

Signed-off-by: srlch <[email protected]>

refactor

59415d3

Signed-off-by: srlch <[email protected]>

srlch requested a review from a team as a code owner November 13, 2024 07:36

fix

1c8fcb3

Signed-off-by: srlch <[email protected]>

srlch force-pushed the support_new_syntax branch from d3c18eb to 1c8fcb3 Compare November 13, 2024 07:38

jaogoy previously approved these changes Nov 13, 2024

View reviewed changes

fix comment

d47c17c

Signed-off-by: srlch <[email protected]>

srlch dismissed jaogoy’s stale review via d47c17c November 13, 2024 08:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Support new syntax for Backup/Restore #52729

[Feature] Support new syntax for Backup/Restore #52729

srlch commented Nov 8, 2024 •

edited by jaogoy

Loading

starrocks-cr bot Nov 8, 2024

starrocks-cr bot Nov 8, 2024

starrocks-cr bot Nov 8, 2024

github-actions bot commented Nov 11, 2024

jaogoy Nov 11, 2024

jaogoy Nov 11, 2024

srlch Nov 13, 2024

srlch Nov 13, 2024

jaogoy Nov 11, 2024

srlch Nov 13, 2024

jaogoy Nov 11, 2024

srlch Nov 13, 2024

jaogoy Nov 11, 2024

srlch Nov 13, 2024

jaogoy Nov 11, 2024

srlch Nov 13, 2024

jaogoy Nov 11, 2024

srlch Nov 13, 2024

jaogoy Nov 11, 2024

srlch Nov 13, 2024

jaogoy Nov 11, 2024

srlch Nov 13, 2024

jaogoy Nov 13, 2024

srlch Nov 13, 2024

jaogoy Nov 13, 2024

srlch Nov 13, 2024

jaogoy Nov 13, 2024

srlch Nov 13, 2024

jaogoy Nov 13, 2024

srlch Nov 13, 2024

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

sonarcloud bot commented Nov 13, 2024


		public class AbstractBackupStmt extends DdlStmt {
		public enum BackupRestoreAllMarker {

		}
		TableRef tableRef = new TableRef(tableName, alias, partitionNames, position);

[Feature] Support new syntax for Backup/Restore #52729

Are you sure you want to change the base?

[Feature] Support new syntax for Backup/Restore #52729

Conversation

srlch commented Nov 8, 2024 • edited by jaogoy Loading

Why we need new syntax for Backup/Restore

New syntax design for Backup/Restore

The behavioral changes in Backup/Restore syntax

What type of PR is this:

Checklist:

Bugfix cherry-pick branch check:

starrocks-cr bot Nov 8, 2024

Choose a reason for hiding this comment

starrocks-cr bot Nov 8, 2024

Choose a reason for hiding this comment

starrocks-cr bot Nov 8, 2024

Choose a reason for hiding this comment

github-actions bot commented Nov 11, 2024

[FE Incremental Coverage Report]

file detail

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 13, 2024

[Java-Extensions Incremental Coverage Report]

github-actions bot commented Nov 13, 2024

[BE Incremental Coverage Report]

sonarcloud bot commented Nov 13, 2024

Quality Gate failed

srlch commented Nov 8, 2024 •

edited by jaogoy

Loading