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. > > 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. > > To test it I attempted to reproduce the problem, using attached > test_revoke.sql in the test. Still, pg_upgrade works fine without any > adjustments. I'd be grateful if you test it some more.
test_revoke.sql has "revoke all on function pg_stat_get_subscription() from test", which does nothing. You need something that causes a REVOKE in pg_dump output. Worked example: === shell script set -x createuser test pg_dump | grep -E 'GRANT|REVOKE' psql -Xc 'revoke all on function pg_stat_get_subscription() from test;' psql -Xc 'revoke all on function pg_stat_get_subscription from test;' pg_dump | grep -E 'GRANT|REVOKE' psql -Xc 'revoke all on function pg_stat_get_subscription from public;' pg_dump | grep -E 'GRANT|REVOKE' === output + createuser test + pg_dump + grep -E 'GRANT|REVOKE' + psql -Xc 'revoke all on function pg_stat_get_subscription() from test;' ERROR: function pg_stat_get_subscription() does not exist + psql -Xc 'revoke all on function pg_stat_get_subscription from test;' REVOKE + pg_dump + grep -E 'GRANT|REVOKE' + psql -Xc 'revoke all on function pg_stat_get_subscription from public;' REVOKE + pg_dump + grep -E 'GRANT|REVOKE' REVOKE ALL ON FUNCTION pg_catalog.pg_stat_get_subscription(subid oid, OUT subid oid, OUT relid oid, OUT pid integer, OUT received_lsn pg_lsn, OUT last_msg_send_time timestamp with time zone, OUT last_msg_receipt_time timestamp with time zone, OUT latest_end_lsn pg_lsn, OUT latest_end_time timestamp with time zone) FROM PUBLIC; That REVOKE is going to fail if the upgrade target cluster lacks the function in question, and your test_rename_catalog_objects_v13 does simulate pg_stat_get_subscription not existing.
