Re: pg_upgrade fails with non-standard ACL

Hello!

On 11/29/19 11:07 AM, Artur Zakirov wrote:
> If Anastasia doesn't mind I'd like to send new version of the patch.
>
> On 2019/11/28 12:29, Artur Zakirov wrote:
>> On 2019/11/27 13:22, Michael Paquier wrote:
>>> Yeah, the actual take is if we want to make the frontend code more
>>> complicated with a large set of SQL queries to check that each object
>>> ACL is modified, which adds an additional maintenance cost in
>>> pg_upgrade.  Or if we want to keep the frontend simple and have more
>>> backend facility to ease cross-catalog lookups for ACLs. Perhaps we
>>> could also live with just checking after the ACLs of functions in the
>>> short term and perhaps it covers most of the cases users would care
>>> about..  That's tricky to conclude about and I am not sure which path
>>> is better in the long-term, but at least it's worth discussing all
>>> possible candidates IMO so as we make sure to not miss anything.
>>
>> I checked what objects changed their signatures between master and 
>> 9.6. I just ran pg_describe_object() for grantable object types, 
>> saved the output into a file and diffed the outputs. It seems that 
>> only functions and table columns changed their signatures. A list of 
>> functions is big and here the list of columns:
>>
>> table pg_attrdef column adsrc
>> table pg_class column relhasoids
>> table pg_class column relhaspkey
>> table pg_constraint column consrc
>> table pg_proc column proisagg
>> table pg_proc column proiswindow
>> table pg_proc column protransform
>>
>> As a result I think in pg_upgrade we could just check functions and 
>> columns signatures. It might simplify the patch. And if something 
>> changes in a future we could fix pg_upgrade too.
> New version of the patch differs from the previous:
> - it doesn't generate script to revoke conflicting permissions (but 
> the patch can be fixed easily)
> - generates file incompatible_objects_for_acl.txt to report which 
> objects changed their signatures
> - uses pg_shdepend and pg_depend catalogs to get objects with custom 
> privileges
> - uses pg_describe_object() to find objects with different signatures
>
> Currently relations, attributes, languages, functions and procedures 
> are scanned. According to the documentation foreign databases, 
> foreign-data wrappers, foreign servers, schemas and tablespaces also 
> support ACLs. But some of them doesn't have entries initialized during 
> initdb, others like schemas and tablespaces didn't change their names. 
> So the patch doesn't scan such objects.
>
> Grigory it would be great if you'll try the patch. I tested it but I 
> could miss something.

I`ve tested the patch on upgrade from 9.5 to master and it works now, 
thank you.
But I think that 'incompatible_objects_for_acl.txt' is not a very 
informative way of reporting to user the source of the problem.
There is no mentions of rolenames that holds permissions, that prevents 
the upgrade, and even if they were, it is still up to user to conjure an 
script to revoke all those grants, which is not a very user-friendly.

I think it should generate 'catalog_procedures.sql' script as in 
previous version with all REVOKE statements, that are required to 
execute for pg_upgrade to work.


-- 
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company