|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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 postgresql.org 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
>> 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
>> 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.
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
|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|