Backup privilege statements as separate entries (#31)

Before patch, all privilege statements placed in metadata file as one pre-data
section entry. `editStatementsRedirectSchema()`, used by schema redirection,
replace only first schema name occurrence in entry. This caused a bug - only
first privilege statement was modifed. This, in turn, caused vaious errors.
`gprestore`, used on same database, applied most of privilege statements to
original schema. Or, when used on different database, caused `schema does not
exist` error.

From now, `GetPrivilegesStatements()` returns a set of separate statements,
which later used as separate entries. `strings.TrimSpace()` was removed as all
statements are already trimmed.

Existed tests were modified as separate entries have different format. New test
shows correct privilege applying.

backup/metadata_globals_test.go

@@ -50,9 +50,9 @@ var _ = Describe("backup/metadata_globals tests", func() {
 				`CREATE DATABASE testdb TEMPLATE template0;`,
 				`COMMENT ON DATABASE testdb IS 'This is a database comment.';`,
 				`ALTER DATABASE testdb OWNER TO testrole;`,
-				`REVOKE ALL ON DATABASE testdb FROM PUBLIC;
-REVOKE ALL ON DATABASE testdb FROM testrole;
-GRANT TEMPORARY,CONNECT ON DATABASE testdb TO testrole;`,
+				`REVOKE ALL ON DATABASE testdb FROM PUBLIC;`,
+				`REVOKE ALL ON DATABASE testdb FROM testrole;`,
+				`GRANT TEMPORARY,CONNECT ON DATABASE testdb TO testrole;`,
 				`SECURITY LABEL FOR dummy ON DATABASE testdb IS 'unclassified';`}
 			testutils.AssertBufferContents(tocfile.GlobalEntries, buffer, expectedStatements...)
 		})
@@ -428,9 +428,9 @@ ALTER ROLE "testRole2" WITH SUPERUSER INHERIT CREATEROLE CREATEDB LOGIN REPLICAT
 				`CREATE TABLESPACE test_tablespace FILESPACE test_filespace;`,
 				`COMMENT ON TABLESPACE test_tablespace IS 'This is a tablespace comment.';`,
 				`ALTER TABLESPACE test_tablespace OWNER TO testrole;`,
-				`REVOKE ALL ON TABLESPACE test_tablespace FROM PUBLIC;
-REVOKE ALL ON TABLESPACE test_tablespace FROM testrole;
-GRANT ALL ON TABLESPACE test_tablespace TO testrole;`,
+				`REVOKE ALL ON TABLESPACE test_tablespace FROM PUBLIC;`,
+				`REVOKE ALL ON TABLESPACE test_tablespace FROM testrole;`,
+				`GRANT ALL ON TABLESPACE test_tablespace TO testrole;`,
 				`SECURITY LABEL FOR dummy ON TABLESPACE test_tablespace IS 'unclassified';`}
 			testutils.AssertBufferContents(tocfile.GlobalEntries, buffer, expectedStatements...)
 

backup/predata_acl.go

@@ -108,8 +108,8 @@ func PrintObjectMetadata(metadataFile *utils.FileWithByteCount, toc *toc.TOC,
 			statements = append(statements, strings.TrimSpace(owner))
 		}
 	}
-	if privileges := metadata.GetPrivilegesStatements(obj.FQN(), entry.ObjectType); privileges != "" {
-		statements = append(statements, strings.TrimSpace(privileges))
+	if privileges := metadata.GetPrivilegesStatements(obj.FQN(), entry.ObjectType); len(privileges) > 0 {
+		statements = append(statements, privileges...);
 	}
 	if securityLabel := metadata.GetSecurityLabelStatement(obj.FQN(), entry.ObjectType); securityLabel != "" {
 		statements = append(statements, strings.TrimSpace(securityLabel))
@@ -138,8 +138,8 @@ func printExtensionFunctionACLs(metadataFile *utils.FileWithByteCount, toc *toc.
 	})
 	statements := make([]string, 0)
 	for _, obj := range objects {
-		if privileges := obj.GetPrivilegesStatements(obj.FQN(), "FUNCTION"); privileges != "" {
-			statements = append(statements, strings.TrimSpace(privileges))
+		if privileges := obj.GetPrivilegesStatements(obj.FQN(), "FUNCTION"); len(privileges) > 0 {
+			statements = append(statements, privileges...);
 			PrintStatements(metadataFile, toc, obj, statements)
 		}
 	}
@@ -290,7 +290,7 @@ func ParseACL(aclStr string) *ACL {
 	return nil
 }
 
-func (obj ObjectMetadata) GetPrivilegesStatements(objectName string, objectType string, columnName ...string) string {
+func (obj ObjectMetadata) GetPrivilegesStatements(objectName string, objectType string, columnName ...string) []string {
 	statements := make([]string, 0)
 	typeStr := fmt.Sprintf("%s ", objectType)
 	if objectType == "VIEW" || objectType == "FOREIGN TABLE" || objectType == "MATERIALIZED VIEW" {
@@ -325,10 +325,7 @@ func (obj ObjectMetadata) GetPrivilegesStatements(objectName string, objectType
 			}
 		}
 	}
-	if len(statements) > 0 {
-		return "\n\n" + strings.Join(statements, "\n")
-	}
-	return ""
+	return statements
 }
 
 func createPrivilegeStrings(acl ACL, objectType string) (string, string) {

backup/predata_acl_test.go

@@ -57,8 +57,14 @@ ALTER TABLE public.tablename OWNER TO testrole;`)
 			testhelper.ExpectRegexp(buffer, `
 
 REVOKE ALL ON TABLE public.tablename FROM PUBLIC;
+
+
 GRANT ALL ON TABLE public.tablename TO anothertestrole;
+
+
 GRANT SELECT,INSERT,UPDATE,DELETE,TRUNCATE,REFERENCES ON TABLE public.tablename TO testrole;
+
+
 GRANT TRIGGER ON TABLE public.tablename TO PUBLIC;`)
 		})
 		It("prints a block of REVOKE and GRANT statements WITH GRANT OPTION", func() {
@@ -67,8 +73,14 @@ GRANT TRIGGER ON TABLE public.tablename TO PUBLIC;`)
 			testhelper.ExpectRegexp(buffer, `
 
 REVOKE ALL ON TABLE public.tablename FROM PUBLIC;
+
+
 GRANT ALL ON TABLE public.tablename TO anothertestrole WITH GRANT OPTION;
+
+
 GRANT SELECT,INSERT,UPDATE,DELETE,TRUNCATE,REFERENCES ON TABLE public.tablename TO testrole WITH GRANT OPTION;
+
+
 GRANT TRIGGER ON TABLE public.tablename TO PUBLIC WITH GRANT OPTION;`)
 		})
 		It("prints a block of REVOKE and GRANT statements, some with WITH GRANT OPTION, some without", func() {
@@ -77,7 +89,11 @@ GRANT TRIGGER ON TABLE public.tablename TO PUBLIC WITH GRANT OPTION;`)
 			testhelper.ExpectRegexp(buffer, `
 
 REVOKE ALL ON TABLE public.tablename FROM PUBLIC;
+
+
 GRANT ALL ON TABLE public.tablename TO anothertestrole;
+
+
 GRANT SELECT,INSERT,UPDATE,DELETE,TRUNCATE,REFERENCES ON TABLE public.tablename TO testrole WITH GRANT OPTION;`)
 		})
 		It("prints both an ALTER TABLE ... OWNER TO statement and a table comment", func() {
@@ -99,9 +115,17 @@ ALTER TABLE public.tablename OWNER TO testrole;
 
 
 REVOKE ALL ON TABLE public.tablename FROM PUBLIC;
+
+
 REVOKE ALL ON TABLE public.tablename FROM testrole;
+
+
 GRANT ALL ON TABLE public.tablename TO anothertestrole;
+
+
 GRANT SELECT,INSERT,UPDATE,DELETE,TRUNCATE,REFERENCES ON TABLE public.tablename TO testrole;
+
+
 GRANT TRIGGER ON TABLE public.tablename TO PUBLIC;`)
 		})
 		It("prints both a block of REVOKE and GRANT statements and a table comment", func() {
@@ -113,8 +137,14 @@ COMMENT ON TABLE public.tablename IS 'This is a table comment.';
 
 
 REVOKE ALL ON TABLE public.tablename FROM PUBLIC;
+
+
 GRANT ALL ON TABLE public.tablename TO anothertestrole;
+
+
 GRANT SELECT,INSERT,UPDATE,DELETE,TRUNCATE,REFERENCES ON TABLE public.tablename TO testrole;
+
+
 GRANT TRIGGER ON TABLE public.tablename TO PUBLIC;`)
 		})
 		It("prints REVOKE and GRANT statements, an ALTER TABLE ... OWNER TO statement, and comments", func() {
@@ -129,9 +159,17 @@ ALTER TABLE public.tablename OWNER TO testrole;
 
 
 REVOKE ALL ON TABLE public.tablename FROM PUBLIC;
+
+
 REVOKE ALL ON TABLE public.tablename FROM testrole;
+
+
 GRANT ALL ON TABLE public.tablename TO anothertestrole;
+
+
 GRANT SELECT,INSERT,UPDATE,DELETE,TRUNCATE,REFERENCES ON TABLE public.tablename TO testrole;
+
+
 GRANT TRIGGER ON TABLE public.tablename TO PUBLIC;`)
 		})
 		It("prints SERVER for ALTER and FOREIGN SERVER for GRANT/REVOKE for a foreign server", func() {
@@ -145,7 +183,11 @@ ALTER SERVER foreignserver OWNER TO testrole;
 
 
 REVOKE ALL ON FOREIGN SERVER foreignserver FROM PUBLIC;
+
+
 REVOKE ALL ON FOREIGN SERVER foreignserver FROM testrole;
+
+
 GRANT ALL ON FOREIGN SERVER foreignserver TO testrole;`)
 		})
 		It("prints FUNCTION for REVOKE and AGGREGATE for ALTER for an aggregate function", func() {
@@ -159,6 +201,8 @@ ALTER AGGREGATE public.testagg(*) OWNER TO testrole;
 
 
 REVOKE ALL ON FUNCTION public.testagg(*) FROM PUBLIC;
+
+
 REVOKE ALL ON FUNCTION public.testagg(*) FROM testrole;`)
 		})
 		Context("Views and sequences have owners", func() {

backup/predata_externals_test.go

@@ -491,9 +491,9 @@ SEGMENT REJECT LIMIT 2 ROWS`)
 			expectedStatements := []string{
 				"CREATE PROTOCOL s3 (readfunc = public.read_fn_s3, writefunc = public.write_fn_s3);",
 				"ALTER PROTOCOL s3 OWNER TO testrole;",
-				`REVOKE ALL ON PROTOCOL s3 FROM PUBLIC;
-REVOKE ALL ON PROTOCOL s3 FROM testrole;
-GRANT ALL ON PROTOCOL s3 TO testrole;`}
+				`REVOKE ALL ON PROTOCOL s3 FROM PUBLIC;`,
+				`REVOKE ALL ON PROTOCOL s3 FROM testrole;`,
+				`GRANT ALL ON PROTOCOL s3 TO testrole;`}
 			testutils.AssertBufferContents(tocfile.PredataEntries, buffer, expectedStatements...)
 		})
 		It("prints a protocol ACL even when the protocol's CREATE statement is skipped", func() {
@@ -511,9 +511,9 @@ GRANT ALL ON PROTOCOL s3 TO testrole;`}
 			backup.PrintCreateExternalProtocolStatement(backupfile, tocfile, protocolUntrustedReadWrite, pgCatalogFuncInfoMap, protoMetadata)
 			expectedStatements := []string{
 				"ALTER PROTOCOL s3 OWNER TO testrole;",
-				`REVOKE ALL ON PROTOCOL s3 FROM PUBLIC;
-REVOKE ALL ON PROTOCOL s3 FROM testrole;
-GRANT ALL ON PROTOCOL s3 TO testrole;`}
+				`REVOKE ALL ON PROTOCOL s3 FROM PUBLIC;`,
+				`REVOKE ALL ON PROTOCOL s3 FROM testrole;`,
+				`GRANT ALL ON PROTOCOL s3 TO testrole;`}
 			testutils.AssertBufferContents(tocfile.PredataEntries, buffer, expectedStatements...)
 		})
 	})

backup/predata_functions_test.go

@@ -61,9 +61,9 @@ $$add_two_ints$$
 LANGUAGE internal%s;`, DEFAULT_PARALLEL),
 					"COMMENT ON FUNCTION public.func_name(integer, integer) IS 'This is a function comment.';",
 					"ALTER FUNCTION public.func_name(integer, integer) OWNER TO testrole;",
-					`REVOKE ALL ON FUNCTION public.func_name(integer, integer) FROM PUBLIC;
-REVOKE ALL ON FUNCTION public.func_name(integer, integer) FROM testrole;
-GRANT ALL ON FUNCTION public.func_name(integer, integer) TO testrole;`,
+					`REVOKE ALL ON FUNCTION public.func_name(integer, integer) FROM PUBLIC;`,
+					`REVOKE ALL ON FUNCTION public.func_name(integer, integer) FROM testrole;`,
+					`GRANT ALL ON FUNCTION public.func_name(integer, integer) TO testrole;`,
 					"SECURITY LABEL FOR dummy ON FUNCTION public.func_name(integer, integer) IS 'unclassified';"}
 				testutils.AssertBufferContents(tocfile.PredataEntries, buffer, expectedStatements...)
 
@@ -825,9 +825,9 @@ ALTER FUNCTION pg_catalog.plperl_validator(oid) OWNER TO testrole;`,
 				// Languages have implicit owners in 4.3, but do not support ALTER OWNER
 				expectedStatements = append(expectedStatements, "ALTER LANGUAGE plpythonu OWNER TO testrole;")
 			}
-			expectedStatements = append(expectedStatements, `REVOKE ALL ON LANGUAGE plpythonu FROM PUBLIC;
-REVOKE ALL ON LANGUAGE plpythonu FROM testrole;
-GRANT ALL ON LANGUAGE plpythonu TO testrole;`,
+			expectedStatements = append(expectedStatements, `REVOKE ALL ON LANGUAGE plpythonu FROM PUBLIC;`,
+				`REVOKE ALL ON LANGUAGE plpythonu FROM testrole;`,
+				`GRANT ALL ON LANGUAGE plpythonu TO testrole;`,
 				"SECURITY LABEL FOR dummy ON LANGUAGE plpythonu IS 'unclassified';")
 
 			testutils.AssertBufferContents(tocfile.PredataEntries, buffer, expectedStatements...)
@@ -853,9 +853,9 @@ GRANT ALL ON LANGUAGE plpythonu TO testrole;`,
 				// Languages have implicit owners in 4.3, but do not support ALTER OWNER
 				expectedStatements = append(expectedStatements, `ALTER LANGUAGE plperl OWNER TO testrole;`)
 			}
-			expectedStatements = append(expectedStatements, `REVOKE ALL ON LANGUAGE plperl FROM PUBLIC;
-REVOKE ALL ON LANGUAGE plperl FROM testrole;
-GRANT ALL ON LANGUAGE plperl TO testrole;`,
+			expectedStatements = append(expectedStatements, `REVOKE ALL ON LANGUAGE plperl FROM PUBLIC;`,
+				`REVOKE ALL ON LANGUAGE plperl FROM testrole;`,
+				`GRANT ALL ON LANGUAGE plperl TO testrole;`,
 				"SECURITY LABEL FOR dummy ON LANGUAGE plperl IS 'unclassified';")
 
 			testutils.AssertBufferContents(tocfile.PredataEntries, buffer, expectedStatements...)

backup/predata_relations.go

@@ -254,7 +254,7 @@ func PrintPostCreateTableStatements(metadataFile *utils.FileWithByteCount, toc *
 		if att.Privileges.Valid {
 			columnMetadata := ObjectMetadata{Privileges: getColumnACL(att.Privileges, att.Kind), Owner: tableMetadata.Owner}
 			columnPrivileges := columnMetadata.GetPrivilegesStatements(table.FQN(), "COLUMN", att.Name)
-			statements = append(statements, strings.TrimSpace(columnPrivileges))
+			statements = append(statements, columnPrivileges...)
 		}
 		if att.SecurityLabel != "" {
 			escapedLabel := utils.EscapeSingleQuotes(att.SecurityLabel)

backup/predata_relations_other_test.go

@@ -217,9 +217,9 @@ SELECT pg_catalog.setval('public.seq_''name', 7, true);`, getSeqDefReplace()))
 SELECT pg_catalog.setval('public.seq_name', 7, true);`, getSeqDefReplace()),
 				"COMMENT ON SEQUENCE public.seq_name IS 'This is a sequence comment.';",
 				fmt.Sprintf("ALTER %s public.seq_name OWNER TO testrole;", keywordReplace),
-				`REVOKE ALL ON SEQUENCE public.seq_name FROM PUBLIC;
-REVOKE ALL ON SEQUENCE public.seq_name FROM testrole;
-GRANT SELECT,USAGE ON SEQUENCE public.seq_name TO testrole;`}
+				`REVOKE ALL ON SEQUENCE public.seq_name FROM PUBLIC;`,
+				`REVOKE ALL ON SEQUENCE public.seq_name FROM testrole;`,
+				`GRANT SELECT,USAGE ON SEQUENCE public.seq_name TO testrole;`}
 			testutils.AssertBufferContents(tocfile.PredataEntries, buffer, expectedEntries...)
 		})
 		It("can print a sequence with privileges WITH GRANT OPTION", func() {
@@ -235,8 +235,8 @@ GRANT SELECT,USAGE ON SEQUENCE public.seq_name TO testrole;`}
 	CACHE 5;
 
 SELECT pg_catalog.setval('public.seq_name', 7, true);`, getSeqDefReplace()),
-				`REVOKE ALL ON SEQUENCE public.seq_name FROM PUBLIC;
-GRANT SELECT,USAGE ON SEQUENCE public.seq_name TO testrole WITH GRANT OPTION;`)
+				`REVOKE ALL ON SEQUENCE public.seq_name FROM PUBLIC;`,
+				`GRANT SELECT,USAGE ON SEQUENCE public.seq_name TO testrole WITH GRANT OPTION;`)
 		})
 		It("prints data_type of the sequence", func() {
 			if connectionPool.Version.Before("7") {
@@ -311,9 +311,9 @@ SELECT pg_catalog.setval('public.seq_name', 10, true);`, getSeqDefReplace()),
 			expectedEntries := []string{"CREATE VIEW shamwow.shazam AS SELECT count(*) FROM pg_tables;",
 				"COMMENT ON VIEW shamwow.shazam IS 'This is a view comment.';",
 				fmt.Sprintf("ALTER %s shamwow.shazam OWNER TO testrole;", keywordReplace),
-				`REVOKE ALL ON shamwow.shazam FROM PUBLIC;
-REVOKE ALL ON shamwow.shazam FROM testrole;
-GRANT ALL ON shamwow.shazam TO testrole;`}
+				`REVOKE ALL ON shamwow.shazam FROM PUBLIC;`,
+				`REVOKE ALL ON shamwow.shazam FROM testrole;`,
+				`GRANT ALL ON shamwow.shazam TO testrole;`}
 
 			if connectionPool.Version.AtLeast("6") {
 				expectedEntries = append(expectedEntries, "SECURITY LABEL FOR dummy ON VIEW shamwow.shazam IS 'unclassified';")

backup/predata_relations_tables_test.go

@@ -617,7 +617,11 @@ ALTER FOREIGN TABLE public.tablename OWNER TO testrole;
 
 
 REVOKE ALL ON public.tablename FROM PUBLIC;
+
+
 REVOKE ALL ON public.tablename FROM testrole;
+
+
 GRANT ALL ON public.tablename TO testrole;
 
 
@@ -654,12 +658,20 @@ ALTER TABLE public.tablename OWNER TO testrole;
 
 
 REVOKE ALL (i) ON TABLE public.tablename FROM PUBLIC;
+
+
 REVOKE ALL (i) ON TABLE public.tablename FROM testrole;
+
+
 GRANT SELECT (i) ON TABLE public.tablename TO testrole;
 
 
 REVOKE ALL (j) ON TABLE public.tablename FROM PUBLIC;
+
+
 REVOKE ALL (j) ON TABLE public.tablename FROM testrole;
+
+
 GRANT ALL (j) ON TABLE public.tablename TO testrole2;`)
 		})
 		It("prints a security group statement on a table column", func() {

backup/predata_shared_test.go

@@ -156,9 +156,9 @@ var _ = Describe("backup/predata_shared tests", func() {
 			expectedStatements := []string{"CREATE SCHEMA schemaname;",
 				"COMMENT ON SCHEMA schemaname IS 'This is a schema comment.';",
 				"ALTER SCHEMA schemaname OWNER TO testrole;",
-				`REVOKE ALL ON SCHEMA schemaname FROM PUBLIC;
-REVOKE ALL ON SCHEMA schemaname FROM testrole;
-GRANT ALL ON SCHEMA schemaname TO testrole;`,
+				`REVOKE ALL ON SCHEMA schemaname FROM PUBLIC;`,
+				`REVOKE ALL ON SCHEMA schemaname FROM testrole;`,
+				`GRANT ALL ON SCHEMA schemaname TO testrole;`,
 				"SECURITY LABEL FOR dummy ON SCHEMA schemaname IS 'unclassified';"}
 			testutils.AssertBufferContents(tocfile.PredataEntries, buffer, expectedStatements...)
 		})

end_to_end/end_to_end_suite_test.go

@@ -1046,6 +1046,44 @@ var _ = Describe("backup and restore end to end tests", func() {
 			assertRelationsCreatedInSchema(restoreConn, "schema2", 0)
 			assertRelationsCreatedInSchema(restoreConn, "fooschema", 0)
 		})
+		It("runs gprestore with --redirect-schema restoring all privileges to the new schema", func() {
+			skipIfOldBackupVersionBefore("1.17.0")
+			testhelper.AssertQueryRuns(backupConn,
+				"DROP ROLE IF EXISTS test_user; CREATE ROLE test_user;")
+			defer testhelper.AssertQueryRuns(backupConn,
+				"DROP ROLE test_user")
+			testhelper.AssertQueryRuns(restoreConn,
+				"DROP SCHEMA IF EXISTS schema_to CASCADE; CREATE SCHEMA schema_to;")
+			defer testhelper.AssertQueryRuns(restoreConn,
+				"DROP SCHEMA schema_to CASCADE")
+			testhelper.AssertQueryRuns(backupConn,
+				"DROP SCHEMA IF EXISTS schema_from CASCADE; CREATE SCHEMA schema_from;")
+			defer testhelper.AssertQueryRuns(backupConn,
+				"DROP SCHEMA schema_from CASCADE")
+			testhelper.AssertQueryRuns(backupConn,
+				"CREATE TABLE schema_from.test_table(i int)")
+			testhelper.AssertQueryRuns(backupConn,
+				"GRANT SELECT ON schema_from.test_table TO test_user")
+
+			timestamp := gpbackup(gpbackupPath, backupHelperPath,
+				"--include-schema", "schema_from")
+
+			gprestore(gprestorePath, restoreHelperPath, timestamp,
+				"--redirect-db", "restoredb",
+				"--include-table", "schema_from.test_table",
+				"--redirect-schema", "schema_to")
+
+			assertRelationsCreatedInSchema(restoreConn, "schema_to", 1)
+
+			// The resulting grantee count is 2 (owner and 'test_user').
+			// '--redirect-schema' should cause all privilege statements to be
+			// edited with new schema name. Restoring should not cause an error
+			// of not existed schema.
+			privCount := dbconn.MustSelectString(restoreConn,
+				`select count(distinct grantee) from information_schema.table_privileges` +
+				` where table_schema = 'schema_to' and table_name = 'test_table'`)
+			Expect(privCount).To(Equal("2"))
+		})
 	})
 	Describe("ACLs for extensions", func() {
 		It("runs gpbackup and gprestores any user defined ACLs on extensions", func() {