Re: pg_upgrade fails with non-standard ACL

On Thu, Feb 11, 2021 at 08:16:30PM +0300, Anastasia Lubennikova wrote:
> On 28.01.2021 09:55, Noah Misch wrote:
> >On Wed, Jan 27, 2021 at 07:32:42PM +0300, Anastasia Lubennikova wrote:
> >>On 27.01.2021 14:21, Noah Misch wrote:
> >>>On Mon, Jan 25, 2021 at 10:14:43PM +0300, Anastasia Lubennikova wrote:
> >>>>1) Could you please clarify, what do you mean by REVOKE failures?
> >>>>
> >>>>I tried following example:
> >>>>
> >>>>Start 9.6 cluster.
> >>>>
> >>>>REVOKE ALL ON function pg_switch_xlog() FROM public;
> >>>>REVOKE ALL ON function pg_switch_xlog() FROM backup;
> >>>>
> >>>>The upgrade to the current master passes with and without patch.
> >>>>It seems that current implementation of pg_upgrade doesn't take into account
> >>>>revoke ACLs.
> >>>I think you can observe it by adding "revoke all on function
> >>>pg_stat_get_subscription from public;" to test_add_acl_to_catalog_objects.sql
> >>>and then rerunning your test script. pg_dump will reproduce that REVOKE,
> >>>which would fail if postgresql.org had removed that function. That's fine, so
> >>>long as a comment mentions the limitation.

I've now tested exactly that. It didn't cause a test script failure, because
pg_upgrade_ACL_test.sh runs "pg_upgrade --check" but does not run the final
pg_upgrade (without --check). The failure happens only without --check. I've
attached a modified version of your test script that reproduces the problem.
It uses a different function, timenow(), so the test does not depend on
test_rename_catalog_objects_v14. The attached script fails with this output:

===
Consult the last few lines of "pg_upgrade_dump_13325.log" for
the probable cause of the failure.
Failure, exiting
===

That log file contains:

===
command: "/tmp/workdir/postgresql_bin_test_new/bin/pg_dump" --host /home/nm/src/pg/backbranch/extra --port 50432 --username nm --schema-only --quote-all-identifiers --binary-upgrade --format=custom --file="pg_upgrade_dump_13325.custom" 'dbname=postgres' >> "pg_upgrade_dump_13325.log" 2>&1

command: "/tmp/workdir/postgresql_bin_test_new/bin/pg_restore" --host /home/nm/src/pg/backbranch/extra --port 50432 --username nm --clean --create --exit-on-error --verbose --dbname template1 "pg_upgrade_dump_13325.custom" >> "pg_upgrade_dump_13325.log" 2>&1
pg_restore: connecting to database for restore
pg_restore: dropping DATABASE PROPERTIES postgres
pg_restore: dropping DATABASE postgres
pg_restore: creating DATABASE "postgres"
pg_restore: connecting to new database "postgres"
pg_restore: creating COMMENT "DATABASE "postgres""
pg_restore: creating DATABASE PROPERTIES "postgres"
pg_restore: connecting to new database "postgres"
pg_restore: creating pg_largeobject "pg_largeobject"
pg_restore: creating ACL "pg_catalog.FUNCTION "timenow"()"
pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 3047; 0 0 ACL FUNCTION "timenow"() nm
pg_restore: error: could not execute query: ERROR: function pg_catalog.timenow() does not exist
Command was: REVOKE ALL ON FUNCTION "pg_catalog"."timenow"() FROM PUBLIC;
===

> >>In the updated patch, I implemented generation of both GRANT ALL and REVOKE
> >>ALL for problematic objects. If I understand it correctly, these calls will
> >>clean object's ACL completely. And I see no harm in doing this, because the
> >>objects don't exist in the new cluster anyway.

Here's fix_system_objects_ACL.sql with your v14 test script:

===
\connect postgres
GRANT ALL ON function pg_catalog.pg_stat_get_subscription(pg_catalog.oid) TO "backup" ;
REVOKE ALL ON function pg_catalog.pg_stat_get_subscription(pg_catalog.oid) FROM "backup" ;
GRANT ALL ON pg_catalog.pg_stat_subscription TO "backup" ;
REVOKE ALL ON pg_catalog.pg_stat_subscription FROM "backup" ;
GRANT ALL ON pg_catalog.pg_subscription TO "backup","test" ;
REVOKE ALL ON pg_catalog.pg_subscription FROM "backup","test" ;
GRANT ALL (subenabled) ON pg_catalog.pg_subscription TO "backup" ;
REVOKE ALL (subenabled) ON pg_catalog.pg_subscription FROM "backup" ;
===

Considering the REVOKE statements, those new GRANT statements have no effect.
To prevent the final pg_upgrade failure, fix_system_objects_ACL.sql would need
"GRANT ALL ON FUNCTION pg_catalog.pg_stat_get_subscription(pg_catalog.oid) TO
PUBLIC;". Alternately, again, I don't mind if this case continues to fail, so
long as a comment mentions the limitation. How would you like to proceed?

One other thing:

> diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
> index 43fc297eb6..f2d593f574 100644
> --- a/src/bin/pg_upgrade/check.c
> +++ b/src/bin/pg_upgrade/check.c
...
> +check_for_changed_signatures(void)
> +{
...
> + /*
> + *
> + * AclInfo array is sorted by obj_ident. This allows us to compare
> + * AclInfo entries with the query result above efficiently.
> + */
> + for (aclnum = 0; aclnum < dbinfo->non_def_acl_arr.nacls; aclnum++)
> + {
> + AclInfo *aclinfo = &dbinfo->non_def_acl_arr.aclinfos[aclnum];
> + bool report = false;
> +
> + while (objnum < ntups)
> + {
> + int ret;
> +
> + ret = strcmp(aclinfo->obj_ident,
> + PQgetvalue(res, objnum, i_obj_ident));

I think this should check obj_type in addition to obj_ident. Otherwise, it
can't distinguish a relation from a type of the same name.

nm