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
pg_upgrade_ACL_test_e2e.sh
Description: Bourne shell script
