From 39c3493fb0bbf2b43cb89a9d533c18740ccb2ce1 Mon Sep 17 00:00:00 2001 From: Jinbao Chen Date: Tue, 7 Jul 2026 11:04:46 -0400 Subject: [PATCH 1/2] Fix external table/FDW metadata queries and update role-grant tests for PG16 backup/queries_externals.go: gp_exttable_fdw represents an unset rejectlimittype as char(-1) ('\377'), not SQL NULL, so GetExternalTableDefinitions was leaking that sentinel through instead of coalescing it to ''. backup/queries_functions.go: GetForeignDataWrappers and GetForeignServers had no ORDER BY, so Postgres could return rows in a nondeterministic order. integration/metadata_globals_*_test.go: PG16 reworked role-grant semantics so an implicit grantor always resolves to the bootstrap superuser rather than the acting (super)user, and an explicit GRANTED BY now requires that role to actually hold ADMIN OPTION. Updated fixtures accordingly, and skipped "handles dropped granter" since PG16's grantor dependency tracking means a role can no longer be dropped while still recorded as another membership's grantor. integration/postdata_create_test.go: updated a hardcoded view-definition golden string to match PG16 ruleutils, which no longer qualifies column references with the table name when only one table is in the FROM clause. --- backup/queries_externals.go | 3 +- backup/queries_functions.go | 6 ++- integration/metadata_globals_create_test.go | 16 ++++++- integration/metadata_globals_queries_test.go | 48 +++++++++++++------- integration/postdata_create_test.go | 6 ++- 5 files changed, 57 insertions(+), 22 deletions(-) diff --git a/backup/queries_externals.go b/backup/queries_externals.go index da9c15bb..940c46b7 100644 --- a/backup/queries_externals.go +++ b/backup/queries_externals.go @@ -61,7 +61,8 @@ func GetExternalTableDefinitions(connectionPool *dbconn.DBConn) map[uint32]Exter e.fmtopts AS formatopts, coalesce(e.command, '') AS command, coalesce(e.rejectlimit, 0) AS rejectlimit, - coalesce(e.rejectlimittype, '') AS rejectlimittype, + -- gp_exttable_fdw represents "not set" as char(-1) i.e. '\377', not SQL NULL + coalesce(nullif(e.rejectlimittype, '\377'), '') AS rejectlimittype, e.logerrors, coalesce('log_errors=persistently' = any(ft.ftoptions), false) AS logerrpersist, pg_encoding_to_char(e.encoding) AS encoding, diff --git a/backup/queries_functions.go b/backup/queries_functions.go index 8fb04069..98c90461 100644 --- a/backup/queries_functions.go +++ b/backup/queries_functions.go @@ -840,7 +840,8 @@ func GetForeignDataWrappers(connectionPool *dbconn.DBConn) []ForeignDataWrapper SELECT pg_catalog.quote_ident(option_name) || ' ' || pg_catalog.quote_literal(option_value) FROM pg_options_to_table(fdwoptions) ORDER BY option_name), ', ') AS options FROM pg_foreign_data_wrapper - WHERE oid >= %d AND %s`, FIRST_NORMAL_OBJECT_ID, ExtensionFilterClause("")) + WHERE oid >= %d AND %s + ORDER BY oid`, FIRST_NORMAL_OBJECT_ID, ExtensionFilterClause("")) err := connectionPool.Select(&results, query) gplog.FatalOnError(err) @@ -889,7 +890,8 @@ func GetForeignServers(connectionPool *dbconn.DBConn) []ForeignServer { FROM pg_options_to_table(fs.srvoptions) ORDER BY option_name), ', ') AS options FROM pg_foreign_server fs LEFT JOIN pg_foreign_data_wrapper fdw ON fdw.oid = srvfdw - WHERE fs.oid >= %d AND %s`, FIRST_NORMAL_OBJECT_ID, ExtensionFilterClause("fs")) + WHERE fs.oid >= %d AND %s + ORDER BY fs.oid`, FIRST_NORMAL_OBJECT_ID, ExtensionFilterClause("fs")) err := connectionPool.Select(&results, query) gplog.FatalOnError(err) diff --git a/integration/metadata_globals_create_test.go b/integration/metadata_globals_create_test.go index 2063c3a1..8cbb57d2 100644 --- a/integration/metadata_globals_create_test.go +++ b/integration/metadata_globals_create_test.go @@ -416,6 +416,11 @@ var _ = Describe("backup integration create statement tests", func() { defer testhelper.AssertQueryRuns(connectionPool, `DROP ROLE testuser`) }) It("grants a role without ADMIN OPTION", func() { + // PG16 requires an explicit GRANTED BY to actually hold + // ADMIN OPTION on the target role; superuser status alone no + // longer suffices. Give testrole that standing before replaying + // the GRANTED BY statement below. + testhelper.AssertQueryRuns(connectionPool, "GRANT usergroup TO testrole WITH ADMIN OPTION") numRoleMembers := len(backup.GetRoleMembers(connectionPool)) expectedRoleMember := backup.RoleMember{Role: "usergroup", Member: "testuser", Grantor: "testrole", IsAdmin: false} backup.PrintRoleMembershipStatements(backupfile, tocfile, []backup.RoleMember{expectedRoleMember}) @@ -424,8 +429,10 @@ var _ = Describe("backup integration create statement tests", func() { resultRoleMembers := backup.GetRoleMembers(connectionPool) Expect(resultRoleMembers).To(HaveLen(numRoleMembers + 1)) + // The setup grant above also makes testrole itself a member of + // usergroup, so match on Member too, not just Role. for _, roleMember := range resultRoleMembers { - if roleMember.Role == "usergroup" { + if roleMember.Role == "usergroup" && roleMember.Member == "testuser" { structmatcher.ExpectStructsToMatch(&expectedRoleMember, &roleMember) return } @@ -433,6 +440,9 @@ var _ = Describe("backup integration create statement tests", func() { Fail("Role 'testuser' is not a member of role 'usergroup'") }) It("grants a role WITH ADMIN OPTION", func() { + // See the "without ADMIN OPTION" case above for why this is needed + // under PG16's stricter GRANTED BY semantics. + testhelper.AssertQueryRuns(connectionPool, "GRANT usergroup TO testrole WITH ADMIN OPTION") numRoleMembers := len(backup.GetRoleMembers(connectionPool)) expectedRoleMember := backup.RoleMember{Role: "usergroup", Member: "testuser", Grantor: "testrole", IsAdmin: true} backup.PrintRoleMembershipStatements(backupfile, tocfile, []backup.RoleMember{expectedRoleMember}) @@ -441,8 +451,10 @@ var _ = Describe("backup integration create statement tests", func() { resultRoleMembers := backup.GetRoleMembers(connectionPool) Expect(resultRoleMembers).To(HaveLen(numRoleMembers + 1)) + // The setup grant above also makes testrole itself a member of + // usergroup, so match on Member too, not just Role. for _, roleMember := range resultRoleMembers { - if roleMember.Role == "usergroup" { + if roleMember.Role == "usergroup" && roleMember.Member == "testuser" { structmatcher.ExpectStructsToMatch(&expectedRoleMember, &roleMember) return } diff --git a/integration/metadata_globals_queries_test.go b/integration/metadata_globals_queries_test.go index d6182e69..259ba66b 100644 --- a/integration/metadata_globals_queries_test.go +++ b/integration/metadata_globals_queries_test.go @@ -5,6 +5,7 @@ import ( "github.com/apache/cloudberry-backup/backup" "github.com/apache/cloudberry-backup/testutils" + "github.com/apache/cloudberry-go-libs/dbconn" "github.com/apache/cloudberry-go-libs/structmatcher" "github.com/apache/cloudberry-go-libs/testhelper" @@ -506,7 +507,13 @@ CREATEEXTTABLE (protocol='gphdfs', type='writable')` }) }) Describe("GetRoleMembers", func() { + // PG16 rewrote role-grant machinery: an implicit (unspecified) grantor + // on a GRANT ROLE always resolves to the bootstrap superuser (the role + // with oid 10, i.e. the role that ran initdb), never to the acting + // superuser, even when the acting role also has the SUPERUSER attribute. + var bootstrapSuperuser string BeforeEach(func() { + bootstrapSuperuser = dbconn.MustSelectString(connectionPool, "SELECT rolname FROM pg_authid WHERE oid = 10") testhelper.AssertQueryRuns(connectionPool, `CREATE ROLE usergroup`) testhelper.AssertQueryRuns(connectionPool, `CREATE ROLE testuser`) }) @@ -516,7 +523,7 @@ CREATEEXTTABLE (protocol='gphdfs', type='writable')` }) It("returns a role without ADMIN OPTION", func() { testhelper.AssertQueryRuns(connectionPool, "GRANT usergroup TO testuser") - expectedRoleMember := backup.RoleMember{Role: "usergroup", Member: "testuser", Grantor: "testrole", IsAdmin: false} + expectedRoleMember := backup.RoleMember{Role: "usergroup", Member: "testuser", Grantor: bootstrapSuperuser, IsAdmin: false} roleMembers := backup.GetRoleMembers(connectionPool) @@ -529,13 +536,20 @@ CREATEEXTTABLE (protocol='gphdfs', type='writable')` Fail("Role 'testuser' is not a member of role 'usergroup'") }) It("returns a role WITH ADMIN OPTION", func() { + // Under PG16, an explicit GRANTED BY requires that role to + // actually hold ADMIN OPTION on the target role; being a superuser + // is no longer sufficient on its own. Grant testrole that standing + // first (itself defaulting to the bootstrap superuser as grantor). + testhelper.AssertQueryRuns(connectionPool, "GRANT usergroup TO testrole WITH ADMIN OPTION") testhelper.AssertQueryRuns(connectionPool, "GRANT usergroup TO testuser WITH ADMIN OPTION GRANTED BY testrole") expectedRoleMember := backup.RoleMember{Role: "usergroup", Member: "testuser", Grantor: "testrole", IsAdmin: true} roleMembers := backup.GetRoleMembers(connectionPool) + // The setup grant above also makes testrole itself a member of + // usergroup, so match on Member too, not just Role. for _, roleMember := range roleMembers { - if roleMember.Role == "usergroup" { + if roleMember.Role == "usergroup" && roleMember.Member == "testuser" { structmatcher.ExpectStructsToMatch(&expectedRoleMember, &roleMember) return } @@ -552,7 +566,7 @@ CREATEEXTTABLE (protocol='gphdfs', type='writable')` testhelper.AssertQueryRuns(connectionPool, `CREATE ROLE "1testuser"`) defer testhelper.AssertQueryRuns(connectionPool, `DROP ROLE "1testuser"`) testhelper.AssertQueryRuns(connectionPool, `GRANT "1usergroup" TO "1testuser"`) - expectedRoleMember := backup.RoleMember{Role: `"1usergroup"`, Member: `"1testuser"`, Grantor: `"1testrole"`, IsAdmin: false} + expectedRoleMember := backup.RoleMember{Role: `"1usergroup"`, Member: `"1testuser"`, Grantor: bootstrapSuperuser, IsAdmin: false} roleMembers := backup.GetRoleMembers(connectionPool) @@ -565,25 +579,27 @@ CREATEEXTTABLE (protocol='gphdfs', type='writable')` Fail(`Role "1testuser" is not a member of role "1usergroup"`) }) It("handles dropped granter", func() { - testhelper.AssertQueryRuns(connectionPool, `CREATE ROLE testdropgranter_role`) - defer testhelper.AssertQueryRuns(connectionPool, `DROP ROLE testdropgranter_role`) - testhelper.AssertQueryRuns(connectionPool, `CREATE ROLE testdropgranter_member`) - defer testhelper.AssertQueryRuns(connectionPool, `DROP ROLE testdropgranter_member`) - testhelper.AssertQueryRuns(connectionPool, `CREATE ROLE testdropgranter_granter`) - testhelper.AssertQueryRuns(connectionPool, `GRANT testdropgranter_role TO testdropgranter_member GRANTED BY testdropgranter_granter`) - testhelper.AssertQueryRuns(connectionPool, `DROP ROLE testdropgranter_granter`) - expectedRoleMember := backup.RoleMember{Role: `testdropgranter_role`, Member: `testdropgranter_member`, Grantor: ``, IsAdmin: false} - - roleMember := backup.GetRoleMembers(connectionPool) - Expect(len(roleMember)).To(Equal(1)) - structmatcher.ExpectStructsToMatch(&expectedRoleMember, &roleMember[0]) + // PG16 tracks the grantor of a role membership as a real dependency + // (pg_shdepend), so a role can no longer be dropped while it is + // still recorded as another membership's grantor - DROP ROLE fails + // with "cannot be dropped because some objects depend on it", and + // REVOKE ADMIN OPTION ... FROM requires CASCADE, which removes the + // dependent membership rather than orphaning its grantor. So the + // on-disk state this test simulates (a membership whose grantor + // role no longer exists) can no longer be produced by live DDL; it + // can still arrive via pg_upgrade from a pre-16 cluster, which is + // exactly the case GetRoleMembers's fallback (a dangling grantor + // oid renders as '' rather than pg_get_userbyid's "unknown" text) + // exists to handle. Skipping rather than asserting on unreachable + // setup. + Skip("PG16 role-grant dependency tracking prevents dropping a role that is still recorded as another membership's grantor; this state can now only arise via pg_upgrade from pre-16 clusters") }) It("handles implicit cast of oid to text", func() { testhelper.AssertQueryRuns(connectionPool, "CREATE OR REPLACE FUNCTION pg_catalog.text(oid) RETURNS text STRICT IMMUTABLE LANGUAGE SQL AS 'SELECT textin(oidout($1));';") testhelper.AssertQueryRuns(connectionPool, "CREATE CAST (oid AS text) WITH FUNCTION pg_catalog.text(oid) AS IMPLICIT;") defer testhelper.AssertQueryRuns(connectionPool, "DROP FUNCTION pg_catalog.text(oid) CASCADE;") testhelper.AssertQueryRuns(connectionPool, "GRANT usergroup TO testuser") - expectedRoleMember := backup.RoleMember{Role: "usergroup", Member: "testuser", Grantor: "testrole", IsAdmin: false} + expectedRoleMember := backup.RoleMember{Role: "usergroup", Member: "testuser", Grantor: bootstrapSuperuser, IsAdmin: false} roleMembers := backup.GetRoleMembers(connectionPool) diff --git a/integration/postdata_create_test.go b/integration/postdata_create_test.go index aba3cfb1..975527a4 100644 --- a/integration/postdata_create_test.go +++ b/integration/postdata_create_test.go @@ -406,7 +406,11 @@ AS $$ BEGIN RAISE EXCEPTION 'exception'; END; $$;`) Schema: "public", Name: "key_dependent_view", Definition: sql.NullString{ - String: " SELECT view_base_table.key,\n (view_base_table.data COLLATE \"C\") AS data\n FROM public.view_base_table\n GROUP BY view_base_table.key;", + // Postgres 16's ruleutils no longer qualifies column + // references with the table name when only one table + // is in the FROM clause (only the FROM entry itself + // still gets schema-qualified). + String: " SELECT key,\n (data COLLATE \"C\") AS data\n FROM public.view_base_table\n GROUP BY key;", Valid: true, }, ColumnDefs: []backup.ColumnDefinition{ From 9edbb875070fcf9ce79a3ecdf1cc14d1886a6304 Mon Sep 17 00:00:00 2001 From: Jinbao Chen Date: Tue, 7 Jul 2026 23:00:28 -0400 Subject: [PATCH 2/2] test --- .github/workflows/cloudberry-backup-ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/cloudberry-backup-ci.yml b/.github/workflows/cloudberry-backup-ci.yml index 5fe3f75e..d97b4231 100644 --- a/.github/workflows/cloudberry-backup-ci.yml +++ b/.github/workflows/cloudberry-backup-ci.yml @@ -53,9 +53,9 @@ permissions: env: CLOUDBERRY_REPO: apache/cloudberry - # Currently targeting REL_2_STABLE (2.x) branch only. + # Currently targeting main branch only. # TODO: Switch back to "main" once Cloudberry 3.x / main branch stabilizes with PG16 support. - CLOUDBERRY_REF: REL_2_STABLE + CLOUDBERRY_REF: main CLOUDBERRY_DIR: cloudberry CLOUDBERRY_BACKUP_DIR: cloudberry-backup