Re: pg_upgrade fails with non-standard ACL

From: Anastasia Lubennikova <a(dot)lubennikova(at)postgrespro(dot)ru>
To: Noah Misch <noah(at)leadboat(dot)com>
Cc: Arthur Zakirov <zaartur(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Grigory Smolkin <g(dot)smolkin(at)postgrespro(dot)ru>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: pg_upgrade fails with non-standard ACL
Date: 2021-01-27 16:32:42
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

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:
The Russian Postgres Company

Attachment Content-Type Size
pg_upgrade_ACL_check_v13.patch text/x-patch 16.0 KB
test_add_acl_to_catalog_objects.sql application/sql 987 bytes application/x-shellscript 3.2 KB
revoke_test.sql application/sql 642 bytes
test_rename_catalog_objects_v13 text/plain 352.2 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Konstantin Knizhnik 2021-01-27 16:39:00 Re: Columns correlation and adaptive query optimization
Previous Message Tom Lane 2021-01-27 16:22:14 Inconsistent function definitions in ECPG's informix.c