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

@@ -1,14 +1,14 @@
 package backup
 
 import (
-	"database/sql"
 	"fmt"
 	"regexp"
 	"sort"
 	"strings"
 
 	"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
 }

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))
 		}

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;

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)
 

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

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() {

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})

integration/predata_table_defs_queries_test.go

@@ -1,14 +1,14 @@
 package integration
 
 import (
-	"database/sql"
 	"fmt"
 
 	"github.com/greenplum-db/gp-common-go-libs/structmatcher"
 	"github.com/greenplum-db/gp-common-go-libs/testhelper"
 	"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))