Repository navigation
feat(whpg19): support whpg19 - #52
Conversation
WHPG19 is based on PostgreSQL 19, so a WHPG19 server reports major
version 19 and falls into gpbackup's ">= 7" query paths -- which were
written against GPDB7/PG12. Seven PostgreSQL majors of catalog churn sit
in between, and five of those changes are visible to gpbackup:
* attstattarget became nullable (PG17), which failed the scan for
every table backed up;
* a column NOT NULL is now also a pg_constraint row of contype 'n'
(PG18), duplicating what the column definition already carries;
* operator class members are recorded as depending on the operator
family rather than the class (PG14), so the pg_depend join found
nothing and classes came back empty;
* "CREATE TYPE ... AS RANGE" creates an internal range->multirange
cast (PG14) that is not independently creatable, so restoring it
errored;
* a type may declare a subscript function (PG14), without which its
ELEMENT cannot be restored at all.
Ported from the WHPG19 catalog-query work on whpg19-catalog-queries
(PR #42, closed unmerged), which was validated against a live WHPG19
cluster. Rebased onto main, which since then has moved to
warehouse-pg/common-go-libs v1.1.0 -- that release already parses the
WHPG19 version banner, so no dependency change is needed here.
PG14+ lets a SQL function carry a pre-parsed, SQL-standard body -- BEGIN ATOMIC ... END, or RETURN expr -- in pg_proc.prosqlbody, and for those functions the server deliberately stores prosrc as the empty string (see the sql_body branch of interpret_AS_clause). Backing up prosrc alone therefore produced a function with no body whatsoever. Deparse the real body with pg_get_function_sqlbody() the way pg_dump does, and prefer it over prosrc wherever it is present. Such a body is not introduced by AS and has to follow the modifiers rather than precede LANGUAGE, so the statement is laid out differently in that case, again mirroring dumpFunc() in pg_dump.c.
PG18 adds virtual generated columns, whose attgenerated code is 'v'. Only 's' was mapped, so a virtual column's code became the empty string -- which reads throughout as "not a generated column". That went wrong twice over: the column definition came out as a plain DEFAULT rather than GENERATED ALWAYS AS ... VIRTUAL, and the column was put into the COPY attribute list even though it has no stored value to copy. Mapping the code fixes both, since ConstructTableAttributesList already excludes every column with a generated code.
PG17 moved a non-libc collation's locale into pg_collation.colllocale and leaves collcollate/collctype NULL, so GetCollations did not merely lose the locale -- selecting those two columns into strings failed outright on any ICU or builtin collation in a backed-up schema. PG17 also adds the 'b' (builtin) provider, which the printer rejected with a fatal error, and ICU collations may carry custom tailoring in collicurules. Select the locale and rules on WHPG19, spell the locale as LOCALE (which is what CREATE COLLATION calls it) when one is present, and accept the builtin provider.
PG15+ lets a database pick a locale provider other than libc, in which case the locale that actually governs it lives in datlocale (plus daticurules for ICU tailoring) rather than in datcollate/datctype. Those two stay populated regardless -- they are BKI_FORCE_NOT_NULL -- so nothing failed; the database just came back as libc under a different collation, quietly. Dump the provider and its locale, and emit LOCALE_PROVIDER with the matching ICU_LOCALE / BUILTIN_LOCALE / ICU_RULES. template0's own provider is now read alongside its encoding, so this is only spelled out for a database that actually diverges from it.
The version-gated code paths are what this port changes, so the unit suites need to run with the connection reporting 19, alongside 5, 6 and 7. All four majors pass.
Bonartze
changed the title
Operator class members: the family lookup replaced the pg_depend one outright, on the strength of a comment that turns out to be wrong -- PG14 did not move all members onto the family, and WHPG19's own pg_dump still joins pg_depend on the opclass. What actually changed is narrower: gistvalidate() puts a GiST opclass's OPERATOR members (and its optional support functions) on the family even when they were declared inline in CREATE OPERATOR CLASS, while btree and hash keep the opclass dependency, as do GiST's required support functions. So merge the two rather than swapping one for the other, and keep the pg_depend attribution wherever it exists, since it is exact. The family lookup now covers only families holding a single opclass -- the shape CREATE OPERATOR CLASS produces when no FAMILY is named. In a shared family nothing in the catalog says which class a family-level member was declared with, and guessing folds in members belonging to a sibling class or added by ALTER OPERATOR FAMILY; those come back as class members on restore, and if the family already held a matching member the restore fails outright. It also gains the schema and extension filters GetOperatorClasses already uses: built-in members are pinned and have no pg_depend rows, so the query this supplements never returned them, and without the filters every backup fetched the whole catalog only to discard it. Casts: the exclusion was applied on every version, where pg_dump gates the equivalent on PG14+, and the comment claimed an anti-join pg_dump does not use. Adopt pg_dump's actual pg_range form and gate it on 19. Databases: the locale-provider guard ignored ICU rules, so a database matching template0's provider and locale but carrying its own tailoring lost ICU_RULES silently. Collation and database locales are interpolated into single-quoted literals, and ICU tailoring uses the apostrophe as its own quoting character, so they need EscapeSingleQuotes like the rest of the package. A table whose every column is generated -- newly constructible with PG18's virtual columns -- yielded an empty COPY column list, which means no list at all, which covers the generated column and fails the restore. Skip its data instead; it has none. A table with no columns is untouched. Also: order GetForeignDataWrappers by name, so two backups of an unchanged database emit its wrappers in the same order; fold the nine copies of the WHPG19 skip into testutils helpers and the duplicated fixture selection into one; and record in the forked e2e fixture that it must be kept in step with gpdb4_objects.sql.
huiliang-liu
left a comment
huiliang-liu
left a comment
There was a problem hiding this comment.
Reviewed against the WHPG19 sources at origin/WHPG-master (19beta1) and WHPG19's own pg_dump.c version gates as a checklist. All nine changes in the PR check out against the catalog headers and backend code, and the unit suites pass locally at both TEST_GPDB_VERSION=19.999.0 and 7.999.0. Nice work on the commit messages and comments.
Cross-checking the rest of the PG12→PG19 delta against pg_dump.c turned up one gap that fails a restore, plus a few that silently drop information. Short list; the ones on files this PR touches are inline, the rest are spelled out below since GitHub only allows inline comments on changed lines.
Blocking
- Partition trigger clones are no longer
tgisinternalon WHPG19, so they get dumped per child and the restore fails with "trigger already exists" (backup/queries_postdata.go:438, details below).
Silently lost on WHPG19, all handled by pg_dump
2. attcompression (PG14) is not backed up; COMPRESSION lz4 columns restore as pglz (inline at queries_table_defs.go).
3. The MAINTAIN privilege (PG17, ACL char m) is dropped by ParseACL, and grantees holding only the old seven privileges get promoted to ALL, which now includes MAINTAIN (backup/predata_acl.go:212, details below).
4. pg_auth_members.inherit_option / set_option (PG16) are not backed up (backup/queries_globals.go:559, details below).
5. Named / NO INHERIT / NOT VALID not-null constraints (PG18) lose those attributes once contype 'n' is excluded; NOT VALID is the one that can fail a restore (inline at queries_shared.go).
6. MULTIRANGE_TYPE_NAME (PG14) is not backed up; a custom multirange name restores as the default (backup/queries_types.go:435, details below).
Factual
7. The opclass-member dependency change landed in PG14, not PG13 (9f9682783 is reachable from REL_14_0 and not from REL_13_0). The comment in queries_operators.go and two commit messages say PG13 (inline).
I'd take (1) and (7) in this PR. (2)–(6) can be follow-up issues if you prefer, but please list them in the PR description so "reviewed the rest of the delta" is accurate about what remains.
backup/queries_postdata.go:438 — blocking
constraintClause := "NOT tgisinternal" is no longer enough on WHPG19. trigger.c creates partition clones with isInternal=false, in_partition=true (the CreateTriggerFiringOn call in the partition loop), so a clone has tgisinternal = false and tgparentid <> 0. On GP7 clones were internal and this filter dropped them; on WHPG19 every child clone is emitted as its own CREATE TRIGGER. On restore the parent's CREATE TRIGGER recreates the clones first, and the explicit child statement then fails with "trigger ... for relation ... already exists". WHPG19 allows row triggers on partitioned tables (only statement triggers and transition tables are blocked), so this is a live path.
pg_dump.c (PG15 branch of getTriggers) uses:
LEFT JOIN pg_catalog.pg_trigger u ON (u.oid = t.tgparentid)
WHERE ((NOT t.tgisinternal AND t.tgparentid = 0) OR t.tgenabled != u.tgenabled)The simplest fix is AND t.tgparentid = 0 on the 19 path; the tgenabled != u.tgenabled half only matters if gpbackup wants to preserve a child that was enabled/disabled independently, which it doesn't do today.
backup/predata_acl.go:212
WHPG19's ACL_ALL_RIGHTS_STR is arwdDxtXUCTcsAm (acl.h:154); m is MAINTAIN (PG17). The for _, char := range permStr switch has no 'm' case and no default, so the character is skipped silently. Two effects: an explicit GRANT MAINTAIN ON t TO r is lost, and createPrivilegeStrings still treats arwdDxt as the full set, so a grantee holding only those seven gets GRANT ALL on restore and thereby gains MAINTAIN. Adding Maintain/MaintainWithGrant to the ACL struct and to the table branch of the "all privileges" check would fix both.
backup/queries_globals.go:559
pg_auth_members has inherit_option and set_option since PG16 (pg_auth_members.h:50,53). Only admin_option is read, so GRANT ... WITH INHERIT FALSE / SET FALSE restores with the defaults. The integration tests in this PR already adapt to the PG16 GRANTED BY semantics, so this seems like the right moment to pick up the other two options as well (pg_dumpall.c gates both on >= 160000).
backup/queries_types.go:435
pg_range.rngmultitypid (PG14, pg_range.h:40) is not read, so a CREATE TYPE ... AS RANGE (..., MULTIRANGE_TYPE_NAME = foo) restores with the default multirange name. pg_dump.c selects format_type(rngmultitypid, NULL) and emits MULTIRANGE_TYPE_NAME when it differs from the default. Functions or columns referring to the custom name will fail on restore.
Partition trigger clones (restore-breaking). trigger.c stores tgisinternal verbatim -- CreateTriggerFiringOn passes isInternal through when it recurses onto a partition child, and records the clone's parent in tgparentid. So on WHPG19 a user row trigger's clones are not internal, NOT tgisinternal let every one of them through, and each child got its own CREATE TRIGGER. The parent's statement already recreates the clones, so the restore failed with "trigger ... already exists". Match on the parent link instead, as getTriggers() does; tgparentid postdates GPDB7, hence the version gate. attcompression (PG14): an explicit COMPRESSION lz4 column silently came back as pglz. Emitted as its own ALTER TABLE ... SET COMPRESSION, which is where pg_dump puts it rather than inline in the column definition. MAINTAIN (PG17, ACL character 'm'): ParseACL skipped the character entirely, so an explicit GRANT MAINTAIN was lost -- and, worse, createPrivilegeStrings still treated arwdDxt as a relation's full set, so a grantee holding only those seven was written as GRANT ALL and came back holding MAINTAIN as well. Both the parse and the "all privileges" check now know about it. The emission is version-gated because the character cannot appear before PG17, which also keeps older majors byte-identical. pg_auth_members.inherit_option / set_option (PG16): GRANT ... WITH INHERIT FALSE / SET FALSE restored with the defaults, quietly widening what the member could do. The options are now built as a list under a single WITH, mirroring pg_dumpall: INHERIT always spelled out, SET only when false. MULTIRANGE_TYPE_NAME (PG14): a custom multirange name restored as the generated default, so anything referring to it failed. Spelled out unconditionally, as dumpRangeType() does, rather than reimplementing makeMultirangeTypeName()'s truncation rules to detect the default. Factual correction: the opclass-member dependency change landed in PG14, not PG13 -- amadjustmembers (9f9682783) is reachable from REL_14_0 and not from REL_13_0. Also folds the one remaining "GPDB7 or later" predicate into testutils alongside the skip helpers, and orders GetForeignDataWrappers by name. Named / NO INHERIT / NOT VALID not-null constraints (PG18) are left to issue #53: reproducing them changes how every column's NOT NULL is rendered, which is a heavily tested path and a poor fit for this PR.
c9cf2a0 to
081ab87
Compare
|
Thanks — that was a genuinely useful pass, and the blocking one was real. (1) Partition trigger clones — fixed. Confirmed in (2), (3), (4), (6) — all taken in this PR rather than deferred; they were each contained enough. (5) — filed as #53 and listed in the PR description, per your offer. It changes how every column's (7) — corrected in the comment and in both commit messages. The branch was force-pushed for that reword; the trees are byte-identical, only the messages changed, so your inline comments should still line up. Description now has a "Second review round" section listing all of the above and what remains. |
The MAINTAIN work broke the WHPG6 and WHPG7 live suites: 13 specs in each, all "Mismatch on field Privileges[N].Maintain". The integration suite compares DefaultACLForType against the ACL a real server hands back, and the fixture was claiming MAINTAIN unconditionally while a GPDB6/7 server has no such privilege to report. It cannot simply be dropped again either: on WHPG19 the server does return 'm' for a relation's full privilege set, so the fixture would then be wrong in the other direction -- and the unit suites need it present at major 19, since createPrivilegeStrings now requires MAINTAIN before it will write GRANT ALL for a relation. So decide it per run. The live connection answers it where there is one, which is the integration case; the unit suites fall through to the environment variable that drives them, because their fixtures are built while Ginkgo is still constructing the spec tree, before any BeforeEach has run and so before a connection exists. The integration suite builds its own connection rather than going through SetupTestEnvironment, so it hands the version over in BeforeSuite, after MustConnect has populated it. The one spec that pairs the fixture with a literal ACL string keeps the seven pre-PG17 privileges and clears Maintain explicitly, so it asserts the same thing on every major.
huiliang-liu
left a comment
huiliang-liu
left a comment
There was a problem hiding this comment.
Two more from re-reading the second round; the rest of it checks out against the WHPG19 sources (SET COMPRESSION as its own ALTER matches pg_dump.c:20133, INHERIT/SET matches dumpRoleMembership(), the fixture version plumbing is sound). Both below are inline.
…play Build MULTIRANGE_TYPE_NAME from pg_type/pg_namespace rather than format_type(). format_type() only schema-qualifies a type the current search_path cannot see, so the qualified output depended on SetSessionGUCs() having set search_path to pg_catalog -- which it does (backup/wrappers.go), but nothing at the query said so, and every other FQN in the package is built from an explicit join. It matters because DefineRange() resolves an unqualified MULTIRANGE_TYPE_NAME against the restore session's search_path rather than the range type's schema. Order role grants so the restore can replay them. PG16+ requires the role named by GRANTED BY to hold ADMIN OPTION on the role being granted at the moment the grant is replayed, so a grant attributed to a non-superuser grantor has to follow that grantor's own admin grant. ORDER BY roleid, member says nothing about that: both rows share the roleid, so it falls to the members' OIDs, and a member that predates its grantor restores first and fails. Reordered the way pg_dumpall's dumpRoleMembership() does it -- repeatedly emit whatever has become replayable, and emit the remainder in catalog order if a pass makes no progress, so nothing is dropped. Gated on 19, where the enforcement starts, leaving older majors byte-identical. The range-type integration specs compare the whole struct, so they now expect the multirange name on 19: PG derives the default by replacing "range" in the range type's name with "multirange" (makeMultirangeTypeName), which is also why the name is spelled out rather than detected.
huiliang-liu
left a comment
huiliang-liu
left a comment
There was a problem hiding this comment.
Thanks for taking both. The multirange change is fine (and see my correction on that thread). One problem with the ordering, inline.
rolsuper was the wrong test. check_role_grantor() (user.c:3306) skips the
ADMIN OPTION requirement for BOOTSTRAP_SUPERUSERID alone:
if (grantorId != BOOTSTRAP_SUPERUSERID &&
select_best_admin(grantorId, roleid) != grantorId)
ereport(ERROR, ... "The grantor must have the ADMIN option on role")
and select_best_admin() is documented as finding an admin "ignoring
super-userness", so a superuser that is not the bootstrap superuser still
needs its own ADMIN OPTION row replayed first. pg_dumpall compares the
recorded grantor against BOOTSTRAP_SUPERUSERID for exactly this reason.
The integration suite was the counter-example: testrole is created
SUPERUSER, yet the BeforeEach in metadata_globals_create_test.go has to
grant it ADMIN OPTION before GRANTED BY testrole will replay. With rolsuper
that grant was waved through and the order fell back to member OID -- the
failure the ordering exists to prevent.
So test the grantor against OID 10 instead, and rename the field to say
what it means. A spec covers the case the old test got wrong; reverting the
predicate fails it.
3 participants
Why
WHPG19 is based on PostgreSQL 19, so a WHPG19 server reports major version 19 and every version gate in this repo puts it on the
>= 7path — paths written against GPDB7, which is PG12. Seven PostgreSQL majors of catalog churn sit in that gap, and a good deal of it is visible to gpbackup.Nothing is needed for version detection:
warehouse-pg/common-go-libsv1.1.0, whichmainalready uses, parses the WHPG19 banner (... (Greenplum Database) 19.0.0 build dev ... WarehousePG) and derives major 19 from it.ValidateGPDBVersionCompatibilityonly enforces a floor, so there is no ceiling to lift either. The work is entirely in the catalog queries and the DDL they feed.What's here
coalesce(a.attstattarget, -1)attstattargetbecame nullable, failing the scan for every table backed upcontype NOT IN ('t','n')NOT NULLis now also apg_constraintrow, duplicating what the column definition carriespg_dependjoin alone returned nothing and those operators were lost (refined in the review round below)CREATE TYPE ... AS RANGEcreates a range→multirange cast that is not independently creatable; restoring it erroredtypsubscript→SUBSCRIPT =ELEMENTwithoutSUBSCRIPT, so such a type could not be restored at allFour further gaps found while reviewing the rest of the PG12→PG19 delta against the
warehouse-pg-nextcatalog headers, each its own commit:BEGIN ATOMIC ... ENDbody inpg_proc.prosqlbodyand storesprosrcas the empty string for those functions (see thesql_bodybranch ofinterpret_AS_clause), so backing upprosrcalone produced a function with no body at all. Now deparsed withpg_get_function_sqlbody(); such a body takes noASand follows the modifiers, mirroringdumpFunc().attgenerated = 'v'was unmapped, which reads everywhere as "not generated" — the column came out as a plainDEFAULTand went into the COPY attribute list despite having no stored value.colllocaleand leavescollcollate/collctypeNULL, soGetCollationsfailed outright on any such collation in a backed-up schema rather than merely losing the locale. Also accepts the new'b'(builtin) provider, which the printer previously treated as fatal, and carriescollicurules.datlocale.datcollate/datctypestay populated regardless (BKI_FORCE_NOT_NULL), so nothing failed — an ICU database just came back as libc, quietly. Now emitsLOCALE_PROVIDERwithICU_LOCALE/BUILTIN_LOCALE/ICU_RULES, spelled out only when it diverges fromtemplate0.Second review round
Reviewing the rest of the PG12→PG19 delta against WHPG19's own
pg_dump.cturned up one restore-breaking gap and four silent ones, all fixed here:trigger.cstorestgisinternalverbatim and records the clone's parent intgparentid, so on WHPG19 a user row trigger's child clones are not internal.NOT tgisinternallet every clone through, each child got its ownCREATE TRIGGER, and since the parent's statement already recreates the clones the restore failed with "trigger ... already exists". Now matches on the parent link, asgetTriggers()does.attcompression(PG14): an explicitCOMPRESSION lz4column came back as pglz. Emitted as its ownALTER TABLE ... SET COMPRESSION, wherepg_dumpputs it.MAINTAIN(PG17, ACL characterm): skipped byParseACL, so an explicit grant was lost — and becausearwdDxtwas still treated as a relation's full set, a grantee holding only those seven was written asGRANT ALLand came back holdingMAINTAINtoo.pg_auth_members.inherit_option/set_option(PG16):GRANT ... WITH INHERIT FALSE/SET FALSErestored with the defaults. Emitted aspg_dumpalldoes —INHERITalways spelled out,SETonly when false.MULTIRANGE_TYPE_NAME(PG14): a custom multirange name restored as the generated default, breaking anything that referred to it. Spelled out unconditionally, asdumpRangeType()does.Also corrected, in the comment and in the two commit messages that said otherwise: the opclass-member dependency change landed in PG14, not PG13 —
amadjustmembers(9f9682783) is reachable fromREL_14_0and not fromREL_13_0.Still outstanding: #53 — named /
NO INHERIT/NOT VALIDnot-null constraints (PG18) are not reproduced oncecontype 'n'is excluded, andNOT VALIDis the one that can fail a restore. Left out because it changes how every column'sNOT NULLis rendered.