Re: pg_upgrade fails with non-standard ACL

Hello,

On 2019/12/05 11:31, Michael Paquier wrote:
> On Wed, Dec 04, 2019 at 06:15:52PM +0900, Arthur Zakirov wrote:
>> Ah, I thought that pg_identify_object() gives properly quoted identity, and
>> it could be used to make SQL script.
>
> It depends on the object type. For columns I can see in your patch
> that you are using a dedicated code path, but I don't think that we
> should assume that there is an exact one-one mapping between the
> object type and the grammar of GRANT/REVOKE for the object type
> supported because both are completely independent things and
> facilities. Take for example foreign data wrappers. Of course for
> this patch this depends if we have system object of the type which
> would be impacted. That's not the case of foreign data wrappers and
> likely it will never be, but there is no way to be sure that this
> won't get broken silently in the future.

I attached new version of the patch. It still uses pg_identify_object(),
I'm not sure about other ways to build identities yet.

It handles relations, their columns, functions, procedure and languages.
There are other GRANT'able objects, like databases, foreign data
wrappers and servers, schemas and tablespaces.

I didn't include handling of databases, schemas and tablespaces because
I don't know yet how to identify if a such object is system object other
than just hard code them. They are not pinned and are not created within
pg_catalog schema.

Foreign data wrappers and servers are not handled too. There are no such
built-in objects, so it is not possible to test them with
"test_rename_catalog_objects.patch". And I'm not sure if they will be
pinned (to test if it is system) if there will be such objects in the
future.

>> Sure! But I'm not sure that I understood the idea. Do you mean the patch
>> which adds a TAP test? It can initialize two clusters, in first it renames
>> some system objects and changes their ACLs. And finally the test runs
>> pg_upgrade which will fail.
>
> A TAP test won't help here because the idea is to create a patch for
> HEAD which willingly introduces changes for system objects, where the
> source binaries have ACLs on object types which are broken on the
> target binaries with the custom patch. That's to make sure that all
> the logic which would get added to pu_upgrade is working correctly.
> Other ideas are welcome.

I added "test_rename_catalog_objects.patch" and
"test_add_acl_to_catalog_objects.sql". So testing steps are following:
- initialize new source instance (e.g. pg v12) and run
"test_add_acl_to_catalog_objects.sql" on it
- apply "pg_upgrade_ACL_check_v6.patch" and
"test_add_acl_to_catalog_objects.sql" for HEAD
- initialize new target instance for HEAD
- run pg_upgrade, it should create revoke_objects.sql file

"test_rename_catalog_objects.patch" should be applied after
"pg_upgrade_ACL_check_v6.patch".

Renamed objects are the following:
- table pg_subscription -> pg_sub
- columns pg_subscription.subenabled -> subactive,
pg_subscription.subconninfo -> subconn
- view pg_stat_subscription -> pg_stat_sub
- columns pg_stat_subscription.received_lsn -> received_location,
pg_stat_subscription.latest_end_lsn -> latest_end_location
- function pg_stat_get_subscription -> pg_stat_get_sub
- language sql -> pgsql

--
Arthur