Re: pg_upgrade fails with non-standard ACL

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.

>
>> 2) As for pinned roles, I think we should fix this behavior, rather than
>> adding a comment. Because such roles can have grants on system objects.
>>
>> In earlier versions of the patch, I gathered ACLs directly from system
>> catalogs: pg_proc.proacl, pg_class.relacl pg_attribute.attacl and
>> pg_type.typacl.
>>
>> The only downside of this approach is that it cannot be easily extended to
>> other object types, as we need to handle every object type separately.
>> I don't think it is a big deal, as we already do it in
>> check_for_changed_signatures()
>>
>> And also the query to gather non-standard ACLs won't look as nice as now,
>> because of the need to parse arrays of aclitems.
>>
>> What do you think?
> Hard to say for certain without seeing the code both ways. I'm not generally
> enthusiastic about adding pg_upgrade code to predict whether the dump will
> fail to restore, because such code will never be as good as just trying the
> restore. The patch has 413 lines of code, which is substantial. I would balk
> if, for example, the patch tripled in size to catch more cases.

Agree.
I attempted to write alternative version, but it seems too complicated.
So I just added a comment about this limitation.

Quoting of table column GRANT/REVOKE statements is fixed in this version.

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company