Re: pg_upgrade fails with non-standard ACL

On 24.01.2021 11:39, Noah Misch wrote:
> On Thu, Jan 21, 2021 at 01:03:58AM +0300, Anastasia Lubennikova wrote:
>> On 03.01.2021 14:29, Noah Misch wrote:
>>> Overall, this patch predicts a subset of cases where pg_dump will emit a
>>> failing GRANT or REVOKE that targets a pg_catalog object.  Can you write a
>>> code comment stating what it does and does not detect?  I think it's okay to
>>> not predict every failure, but let's record the intended coverage.  Here are a
>>> few examples I know; there may be more.  The above query only finds GRANTs to
>>> non-pinned roles.  pg_dump dumps the following, but the above query doesn't
>>> find them:
>>>
>>>    REVOKE ALL ON FUNCTION pg_last_wal_replay_lsn FROM public;
>>>    GRANT EXECUTE ON FUNCTION pg_reload_conf() TO pg_signal_backend;
> I see a new comment listing object types.  Please also explain the lack of
> preventing REVOKE failures (first example query above) and the limitation
> around non-pinned roles (second example).
>

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.

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?

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