From f65a1893cc3fd0ca5db968e649d051ab19c40c4d Mon Sep 17 00:00:00 2001 From: Roman Eskin Date: Wed, 22 May 2024 00:03:30 +1000 Subject: [PATCH] Fix duplicated column definition in the gpbackup metadata (#83) Problem description: If a column in a table had privileges granted for two (or more) users, gpbackup created metadata file, where this column was duplicated. Cause: The SQL query, that is used to retrieve information about all table columns in the function 'GetColumnDefinitions', had the 'unnest' function applied to the array of column-level access privileges from the 'pg_catalog.pg_attribute'. Because of the 'unnest', if there were several aclitems for a column, the result of the query had several rows returned for this column. But gpbackup treated all these rows as results for separate columns, causing duplication in the metadata. Fix: 'unnest' is removed in the query, 'attacl' is returned as is. Thus, for each column, there is only one row in the result of the query. Now, the field 'Privileges' in the 'ColumnDefinition' struct contains all column-level access privileges in a slice of strings ('pq.StringArray'). And later, in 'getColumnACL', this slice, where each string contains a separate aclitem, is iterated and each item is parsed as before. Plus, the 'Kind' field is removed from the 'ColumnDefinition' struct. --- backup/predata_acl.go | 18 ++++++------- backup/predata_relations.go | 4 +-- backup/predata_relations_tables_test.go | 6 +++-- backup/queries_relation_test.go | 2 +- backup/queries_table_defs.go | 22 +++------------- end_to_end/end_to_end_suite_test.go | 25 +++++++++++++++++++ integration/predata_relations_create_test.go | 3 ++- .../predata_table_defs_queries_test.go | 13 +++++++--- 8 files changed, 54 insertions(+), 39 deletions(-) diff --git a/backup/predata_acl.go b/backup/predata_acl.go index 2d0fe3bf1..330efd8eb 100644 --- a/backup/predata_acl.go +++ b/backup/predata_acl.go @@ -1,7 +1,6 @@ package backup import ( - "database/sql" "fmt" "regexp" "sort" @@ -9,6 +8,7 @@ import ( "github.com/greenplum-db/gpbackup/toc" "github.com/greenplum-db/gpbackup/utils" + "github.com/lib/pq" ) var ACLRegex = regexp.MustCompile(`^(.*)=([a-zA-Z\*]*)/(.*)$`) @@ -186,17 +186,13 @@ func ConstructMetadataMap(results []MetadataQueryStruct) MetadataMap { return metadataMap } -func getColumnACL(privileges sql.NullString, kind string) []ACL { - privilegesStr := "" - if kind == "Empty" { - privilegesStr = "GRANTEE=/GRANTOR" - } else if privileges.Valid { - privilegesStr = privileges.String - } +func getColumnACL(privileges pq.StringArray) []ACL { columnMetadata := make([]ACL, 0) - acl := ParseACL(privilegesStr) - if acl != nil { - columnMetadata = append(columnMetadata, *acl) + for _, privilege := range privileges { + acl := ParseACL(privilege) + if acl != nil { + columnMetadata = append(columnMetadata, *acl) + } } return columnMetadata } diff --git a/backup/predata_relations.go b/backup/predata_relations.go index 932b0d657..f18fae5c2 100644 --- a/backup/predata_relations.go +++ b/backup/predata_relations.go @@ -282,8 +282,8 @@ func PrintPostCreateTableStatements(metadataFile *utils.FileWithByteCount, objTo escapedComment := utils.EscapeSingleQuotes(att.Comment) statements = append(statements, fmt.Sprintf("COMMENT ON COLUMN %s.%s IS '%s';", table.FQN(), att.Name, escapedComment)) } - if att.Privileges.Valid { - columnMetadata := ObjectMetadata{Privileges: getColumnACL(att.Privileges, att.Kind), Owner: tableMetadata.Owner} + if len(att.Privileges) > 0 { + columnMetadata := ObjectMetadata{Privileges: getColumnACL(att.Privileges), Owner: tableMetadata.Owner} columnPrivileges := columnMetadata.GetPrivilegesStatements(table.FQN(), toc.OBJ_COLUMN, att.Name) statements = append(statements, strings.TrimSpace(columnPrivileges)) } diff --git a/backup/predata_relations_tables_test.go b/backup/predata_relations_tables_test.go index 942fc4b7c..2c82af050 100644 --- a/backup/predata_relations_tables_test.go +++ b/backup/predata_relations_tables_test.go @@ -7,6 +7,7 @@ import ( "github.com/greenplum-db/gpbackup/backup" "github.com/greenplum-db/gpbackup/testutils" "github.com/greenplum-db/gpbackup/toc" + "github.com/lib/pq" . "github.com/onsi/ginkgo/v2" ) @@ -653,8 +654,8 @@ COMMENT ON COLUMN public.tablename.i IS 'This is a column comment.'; COMMENT ON COLUMN public.tablename.j IS 'This is another column comment.';`) }) It("prints a GRANT statement on a table column", func() { - privilegesColumnOne := backup.ColumnDefinition{Oid: 0, Num: 1, Name: "i", Type: "integer", StatTarget: -1, Privileges: sql.NullString{String: "testrole=r/testrole", Valid: true}} - privilegesColumnTwo := backup.ColumnDefinition{Oid: 1, Num: 2, Name: "j", Type: "character varying(20)", StatTarget: -1, Privileges: sql.NullString{String: "testrole2=arwx/testrole2", Valid: true}} + privilegesColumnOne := backup.ColumnDefinition{Oid: 0, Num: 1, Name: "i", Type: "integer", StatTarget: -1, Privileges: pq.StringArray{"testrole=r/testrole", `" test, role "=r/testrole`}} + privilegesColumnTwo := backup.ColumnDefinition{Oid: 1, Num: 2, Name: "j", Type: "character varying(20)", StatTarget: -1, Privileges: pq.StringArray{"testrole2=arwx/testrole2"}} col := []backup.ColumnDefinition{privilegesColumnOne, privilegesColumnTwo} testTable.ColumnDefs = col tableMetadata := backup.ObjectMetadata{Owner: "testrole"} @@ -667,6 +668,7 @@ 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; +GRANT SELECT (i) ON TABLE public.tablename TO " test, role "; REVOKE ALL (j) ON TABLE public.tablename FROM PUBLIC; diff --git a/backup/queries_relation_test.go b/backup/queries_relation_test.go index a52f6acf1..73321176f 100644 --- a/backup/queries_relation_test.go +++ b/backup/queries_relation_test.go @@ -38,7 +38,7 @@ var _ = Describe("backup internal tests", func() { }) Describe("GetAllViews", func() { It("GetAllViews properly handles NULL view definitions", func() { - columnDefHeader := []string{"attrelid", "attnum", "name", "attnotnull", "atthasdef", "type", "encoding", "attstattarget", "storagetype", "defaultval", "comment", "privileges", "kind", "options", "fdwoptions", "collation", "securitylabelprovider", "securitylabel", "attgenerated", "isinherited"} + columnDefHeader := []string{"attrelid", "attnum", "name", "attnotnull", "atthasdef", "type", "encoding", "attstattarget", "storagetype", "defaultval", "comment", "privileges", "options", "fdwoptions", "collation", "securitylabelprovider", "securitylabel", "attgenerated", "isinherited"} fakeColumnDef := sqlmock.NewRows(columnDefHeader) mock.ExpectQuery(`SELECT (.*)`).WillReturnRows(fakeColumnDef) diff --git a/backup/queries_table_defs.go b/backup/queries_table_defs.go index 84a937d8e..dbf6a7c33 100644 --- a/backup/queries_table_defs.go +++ b/backup/queries_table_defs.go @@ -14,6 +14,7 @@ import ( "github.com/greenplum-db/gp-common-go-libs/gplog" "github.com/greenplum-db/gpbackup/options" "github.com/greenplum-db/gpbackup/toc" + "github.com/lib/pq" ) type Table struct { @@ -230,8 +231,7 @@ type ColumnDefinition struct { StorageType string DefaultVal string Comment string - Privileges sql.NullString - Kind string + Privileges pq.StringArray Options string FdwOptions string Collation string @@ -302,15 +302,7 @@ func GetColumnDefinitions(connectionPool *dbconn.DBConn) map[uint32][]ColumnDefi CASE WHEN a.attstorage != t.typstorage THEN a.attstorage ELSE '' END AS storagetype, coalesce('('||pg_catalog.pg_get_expr(ad.adbin, ad.adrelid)||')', '') AS defaultval, coalesce(d.description, '') AS comment, - CASE - WHEN a.attacl IS NULL THEN NULL - WHEN array_upper(a.attacl, 1) = 0 THEN a.attacl[0] - ELSE unnest(a.attacl) END AS privileges, - CASE - WHEN a.attacl IS NULL THEN '' - WHEN array_upper(a.attacl, 1) = 0 THEN 'Empty' - ELSE '' - END AS kind, + a.attacl AS privileges, coalesce(pg_catalog.array_to_string(a.attoptions, ','), '') AS options, coalesce(array_to_string(ARRAY(SELECT option_name || ' ' || quote_literal(option_value) FROM pg_options_to_table(attfdwoptions) ORDER BY option_name), ', '), '') AS fdwoptions, CASE WHEN a.attcollation <> t.typcollation THEN quote_ident(cn.nspname) || '.' || quote_ident(coll.collname) ELSE '' END AS collation, @@ -353,12 +345,7 @@ func GetColumnDefinitions(connectionPool *dbconn.DBConn) map[uint32][]ColumnDefi coalesce('('||pg_catalog.pg_get_expr(ad.adbin, ad.adrelid)||')', '') AS defaultval, coalesce(d.description, '') AS comment, a.attgenerated, - ljl_unnest AS privileges, - CASE - WHEN a.attacl IS NULL THEN '' - WHEN array_upper(a.attacl, 1) = 0 THEN 'Empty' - ELSE '' - END AS kind, + a.attacl AS privileges, coalesce(pg_catalog.array_to_string(a.attoptions, ','), '') AS options, coalesce(array_to_string(ARRAY(SELECT option_name || ' ' || quote_literal(option_value) FROM pg_options_to_table(attfdwoptions) ORDER BY option_name), ', '), '') AS fdwoptions, CASE WHEN a.attcollation <> t.typcollation THEN quote_ident(cn.nspname) || '.' || quote_ident(coll.collname) ELSE '' END AS collation, @@ -375,7 +362,6 @@ func GetColumnDefinitions(connectionPool *dbconn.DBConn) map[uint32][]ColumnDefi LEFT JOIN pg_collation coll ON a.attcollation = coll.oid LEFT JOIN pg_namespace cn ON coll.collnamespace = cn.oid LEFT JOIN pg_seclabel sec ON sec.objoid = a.attrelid AND sec.classoid = 'pg_class'::regclass AND sec.objsubid = a.attnum - LEFT JOIN LATERAL unnest(a.attacl) ljl_unnest ON a.attacl IS NOT NULL AND array_length(a.attacl, 1) != 0 WHERE %s AND c.reltype <> 0 AND a.attnum > 0::pg_catalog.int2 diff --git a/end_to_end/end_to_end_suite_test.go b/end_to_end/end_to_end_suite_test.go index c06e99425..556371c1e 100644 --- a/end_to_end/end_to_end_suite_test.go +++ b/end_to_end/end_to_end_suite_test.go @@ -1982,6 +1982,31 @@ LANGUAGE plpgsql NO SQL;`) _, err := gprestoreCmd.CombinedOutput() Expect(err).ToNot(HaveOccurred()) }) + It("checks access privileges for multiple users on a column", func() { + testutils.SkipIfBefore6(backupConn) + if useOldBackupVersion { + Skip("This test is not needed for old backup versions") + } + testhelper.AssertQueryRuns(backupConn, `CREATE TABLE public.t1 (a int) DISTRIBUTED BY (a);`) + testhelper.AssertQueryRuns(backupConn, `CREATE USER user1;`) + testhelper.AssertQueryRuns(backupConn, `CREATE USER user2;`) + testhelper.AssertQueryRuns(backupConn, `CREATE USER "user, 3 ";`) + testhelper.AssertQueryRuns(backupConn, `GRANT SELECT (a) on public.t1 to user1;`) + testhelper.AssertQueryRuns(backupConn, `GRANT SELECT (a) on public.t1 to user2;`) + testhelper.AssertQueryRuns(backupConn, `GRANT SELECT (a) on public.t1 to "user, 3 ";`) + defer testhelper.AssertQueryRuns(backupConn, `DROP USER user1;`) + defer testhelper.AssertQueryRuns(backupConn, `DROP USER user2;`) + defer testhelper.AssertQueryRuns(backupConn, `DROP USER "user, 3 ";`) + defer testhelper.AssertQueryRuns(backupConn, `DROP TABLE public.t1;`) + + timestamp := gpbackup(gpbackupPath, backupHelperPath, "--backup-dir", backupDir) + + contents := string(getMetdataFileContents(backupDir, timestamp, "metadata.sql")) + Expect(contents).To(ContainSubstring("CREATE TABLE public.t1 (\n\ta integer\n) DISTRIBUTED BY (a);")) + Expect(contents).To(ContainSubstring(`GRANT SELECT (a) ON TABLE public.t1 TO user1;`)) + Expect(contents).To(ContainSubstring(`GRANT SELECT (a) ON TABLE public.t1 TO user2;`)) + Expect(contents).To(ContainSubstring(`GRANT SELECT (a) ON TABLE public.t1 TO "user, 3 ";`)) + }) }) }) Describe("Restore to a different-sized cluster", FlakeAttempts(5), func() { diff --git a/integration/predata_relations_create_test.go b/integration/predata_relations_create_test.go index f2f6f2416..402af033e 100644 --- a/integration/predata_relations_create_test.go +++ b/integration/predata_relations_create_test.go @@ -10,6 +10,7 @@ import ( "github.com/greenplum-db/gpbackup/backup" "github.com/greenplum-db/gpbackup/testutils" "github.com/greenplum-db/gpbackup/toc" + "github.com/lib/pq" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -422,7 +423,7 @@ SET SUBPARTITION TEMPLATE ` + ` }) It("prints column level privileges", func() { testutils.SkipIfBefore6(connectionPool) - privilegesColumnOne := backup.ColumnDefinition{Oid: 0, Num: 1, Name: "i", Type: "integer", StatTarget: -1, Privileges: sql.NullString{String: "testrole=r/testrole", Valid: true}} + privilegesColumnOne := backup.ColumnDefinition{Oid: 0, Num: 1, Name: "i", Type: "integer", StatTarget: -1, Privileges: pq.StringArray{"testrole=r/testrole", "anothertestrole=r/testrole"}} tableMetadata.Owner = "testrole" testTable.ColumnDefs = []backup.ColumnDefinition{privilegesColumnOne} backup.PrintPostCreateTableStatements(backupfile, tocfile, testTable, tableMetadata, []uint32{0, 0}) diff --git a/integration/predata_table_defs_queries_test.go b/integration/predata_table_defs_queries_test.go index 0861ca63a..e8fe06a28 100644 --- a/integration/predata_table_defs_queries_test.go +++ b/integration/predata_table_defs_queries_test.go @@ -1,7 +1,6 @@ package integration import ( - "database/sql" "fmt" "github.com/greenplum-db/gp-common-go-libs/structmatcher" @@ -9,6 +8,7 @@ import ( "github.com/greenplum-db/gpbackup/backup" "github.com/greenplum-db/gpbackup/options" "github.com/greenplum-db/gpbackup/testutils" + "github.com/lib/pq" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -69,17 +69,22 @@ PARTITION BY RANGE (year) testhelper.AssertQueryRuns(connectionPool, "ALTER TABLE public.atttable DROP COLUMN b") testhelper.AssertQueryRuns(connectionPool, "ALTER TABLE public.atttable ALTER COLUMN e SET STORAGE PLAIN") oid := testutils.OidFromObjectName(connectionPool, "public", "atttable", backup.TYPE_RELATION) - privileges := sql.NullString{String: "", Valid: false} + privileges := make(pq.StringArray, 0) + privilegesMulti := make(pq.StringArray, 0) if connectionPool.Version.AtLeast("6") { testhelper.AssertQueryRuns(connectionPool, "GRANT SELECT (c, d) ON TABLE public.atttable TO testrole") - privileges = sql.NullString{String: "testrole=r/testrole", Valid: true} + privileges = append(privileges, "testrole=r/testrole") + testhelper.AssertQueryRuns(connectionPool, "GRANT SELECT (e) ON TABLE public.atttable TO testrole") + testhelper.AssertQueryRuns(connectionPool, "GRANT SELECT (e) ON TABLE public.atttable TO anothertestrole") + privilegesMulti = append(privilegesMulti, "testrole=r/testrole") + privilegesMulti = append(privilegesMulti, "anothertestrole=r/testrole") } tableAtts := backup.GetColumnDefinitions(connectionPool)[oid] columnA := backup.ColumnDefinition{Oid: 0, Num: 1, Name: "a", NotNull: false, HasDefault: false, Type: "double precision", Encoding: "", StatTarget: -1, StorageType: "", DefaultVal: "", Comment: "att comment"} columnC := backup.ColumnDefinition{Oid: 0, Num: 3, Name: "c", NotNull: true, HasDefault: false, Type: "text", Encoding: "", StatTarget: -1, StorageType: "", DefaultVal: "", Comment: "", Privileges: privileges} columnD := backup.ColumnDefinition{Oid: 0, Num: 4, Name: "d", NotNull: false, HasDefault: true, Type: "integer", Encoding: "", StatTarget: -1, StorageType: "", DefaultVal: "(5)", Comment: "", Privileges: privileges} - columnE := backup.ColumnDefinition{Oid: 0, Num: 5, Name: "e", NotNull: false, HasDefault: false, Type: "text", Encoding: "", StatTarget: -1, StorageType: "PLAIN", DefaultVal: "", Comment: ""} + columnE := backup.ColumnDefinition{Oid: 0, Num: 5, Name: "e", NotNull: false, HasDefault: false, Type: "text", Encoding: "", StatTarget: -1, StorageType: "PLAIN", DefaultVal: "", Comment: "", Privileges: privilegesMulti} Expect(tableAtts).To(HaveLen(4))