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
