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
