Re: Test instability when pg_dump orders by OID

On Sat, Aug 23, 2025 at 07:45:05AM -0700, Noah Misch wrote:
> On Fri, Aug 22, 2025 at 10:20:19PM -0700, Noah Misch wrote:
> > Pushed as b61a5c4 with a few other
> > cosmetic changes. Thanks.
>
> TestUpgradeXversion fails:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=copperhead&dt=2025-08-23%2007%3A34%3A38
>
> --- /home/pgbf/buildroot/upgrade.copperhead/REL_18_STABLE/origin-REL_16_STABLE.sql.fixed 2025-08-23 10:28:16.464887433 +0200
> +++ /home/pgbf/buildroot/upgrade.copperhead/REL_18_STABLE/converted-REL_16_STABLE-to-REL_18_STABLE.sql.fixed 2025-08-23 10:28:16.508887289 +0200
> @@ -606490,13 +606490,13 @@
> --
> -- Name: DEFAULT PRIVILEGES FOR TABLES; Type: DEFAULT ACL; Schema: -; Owner: pg_read_all_settings
> --
> -ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings REVOKE SELECT,INSERT,REFERENCES,DELETE,TRIGGER,TRUNCATE,UPDATE ON TABLES FROM pg_read_all_settings;
> -ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings GRANT SELECT,REFERENCES,DELETE,TRIGGER,TRUNCATE,UPDATE ON TABLES TO pg_read_all_settings;
> +ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings REVOKE ALL ON TABLES FROM pg_read_all_settings;
> +ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings GRANT SELECT,REFERENCES,DELETE,TRIGGER,TRUNCATE,MAINTAIN,UPDATE ON TABLES TO pg_read_all_settings;
>
>
> Crossing the boundary of the MAINTAIN privilege existing seems relevant. Will
> fix. (My checklist did tell me to do a local run of TestUpgradeXversion. I
> skipped it, betting this patch wouldn't break that test. I lost that bet.)

I've pushed commit ad44124 to fix this by changing "REVOKE INSERT ON TABLES"
to "REVOKE USAGE ON TYPES". Types have the same one privilege in all
supported versions, so they avoid the problem. An alternative was to GRANT to
a different role, not REVOKE from the owner role.

I noticed that the buildfarm diff revealed suboptimal pg_dump behaviors
involving v17+ pg_dump dumping a v16- origin server. I failed to resist
studying that. While I plan not to work on these myself, see below for what I
found. If someone works on these, that probably deserves its own thread:

==== Dump+reload+dump sees a schema diff

Consider the sequence:

1. start a v16 server
2. run the commands commit b61a5c4 added to privileges.sql
3. dump the v16 server w/ v17 pg_dump
includes: REVOKE (all but MAINTAIN); GRANT (all but INSERT and MAINTAIN)
4. start a v17 server
5. restore the dump to the v17 server
6. dump the v17 server
includes: REVOKE ALL; GRANT (all but INSERT)

The dump from (3) differs from the dump in (6), hence the buildfarm failure.
Both dumps correctly reproduce catalog state, but it's not great to vary
output like this.

buildACLCommands() operates at the aclitem level. If it gets these args:

acls = defaclacl = {pg_signal_backend=rwdDxt/pg_signal_backend}
baseacls = acldefault = {pg_signal_backend=arwdDxt/pg_signal_backend}

then it revokes the 7 "baseacls" privileges and grants the 6 "acls"
privileges. By diffing individual ai_privs bits instead of whole aclitem
values, it could deduce that REVOKE INSERT suffices. In other words, do the
equivalent of exploding each aclitem into individual privilege records before
diffing to decide what to GRANT and what to REVOKE:

# defaclacl
(pg_signal_backend,r,$grantor)
...

# acldefault
(pg_signal_backend,a,$grantor)
(pg_signal_backend,r,$grantor)
...

I think that would also make the dump output do the right thing in e.g. v17
pg_dump migrating a v17 origin to a v16 destination. (That's not guaranteed
to work, but we don't offer a better-supported downgrade path.) We could then
re-add the test from commit b61a5c4. There could easily be corner cases I'm
not considering; the strategy may be a dead end.

==== Never emits {REVOKE|GRANT} ALL ON {TABLE|TABLES}

Tactically, that's because v17 parseACLItem() never finds the MAINTAIN bit in
a v16- aclitem. A v16 GRANT ALL ON TABLE becomes a GRANT (all but MAINTAIN)
ON TABLE when migrated to v17. The prior debut of new relation ai_privs bits,
REFERENCES etc., came in v7.2. Unlike MAINTAIN, v7.2 commit 1b68bcfad checked
the v7.2+ bits only for v7.2+ origin servers. A v7.1 aclitem that had every
v7.1-era privilege (arwR = SELECT,UPDATE/DELETE,INSERT,RULE) would also get
v7.2 REFERENCES. Lack of any v7.1-era privilege would mean no REFERENCES.

The v7.2 approach would be suboptimal if a new bit is more powerful than all
existing bits for the object type. For example, FOREIGN SERVER, LANGUAGE,
TYPE, etc. have just USAGE. If v19 were to introduce any other privilege for
those, v18->v19 restore shouldn't emit GRANT ALL for those. (v7.2 was okay,
because v7.1 RULE was roughly as powerful as v7.2 REFERENCES.)

Given the 11 months without complaints, changing the MAINTAIN treatment now is
likely riskier than leaving it as-is. If I had a realized this outcome before
the v17 release, though, I would have recommended the v7.2 approach instead.
(MAINTAIN is strictly less powerful than REFERENCES, so the last paragraph's
caveat does not apply.)

==== Outdated comment in buildACLCommands

(This is not specific to v17+.)

/*
* At this point we have issued REVOKE statements for all initial and
* default privileges that are no longer present on the object, so we are
* almost ready to GRANT the privileges listed in grantitems[].
*
* We still need some hacking though to cover the case where new default
* public privileges are added in new versions: the REVOKE ALL will revoke
* them, leading to behavior different from what the old version had,
* which is generally not what's wanted. So add back default privs if the
* source database is too old to have had that particular priv. (As of
* right now, no such cases exist in supported versions.)
*/

This originated in commit 2ee56b6a3 (2007-01), to handle v8.2 adding privilege
CONNECT ON DATABASE. It's no longer just "new default public privileges" that
could need code here. Any version-dependent "baseacls" (world_default,
owner_default, or pg_init_privs) could entail code here. For example, if
MAINTAIN privilege had used the v7.2 pg_dump approach (see heading "Never
emits {REVOKE|GRANT} ALL ON {TABLE|TABLES}"), then a GRANT MAINTAIN for table
owners would belong here, to compensate for the v17 owner_default change.

The "diffing individual privileges" approach potentially removes the need for
code here.

If anyone seeks more details on the above, let me know.