Re: pg_upgrade fails with non-standard ACL

On Mon, Jan 25, 2021 at 10:14:43PM +0300, Anastasia Lubennikova wrote:
> 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.

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.

> 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.