Re: pg_upgrade fails with non-standard ACL

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.