Re: pg_upgrade fails with non-standard ACL

From: Anastasia Lubennikova <a(dot)lubennikova(at)postgrespro(dot)ru>
To: Arthur Zakirov <zaartur(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>
Cc: 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: 2020-03-24 17:01:07
Message-ID: eb36a1d6-e220-1c6b-4057-50246f503ca7@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 17.12.2019 11:10, Arthur Zakirov wrote:
> 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.
>
Michael, do I understand it correctly that your concerns about
pg_identify_object() relate only to the revoke sql script?

Now, I tend to agree that we should split this patch into two separate
parts, to make it simpler.
The first patch is to find affected objects and print warnings and the
second is to generate script.

>
> 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
>
I've tried to test it. Script is attached.
Described test case works. If a new cluster contains renamed objects,
/pg_upgrade --check/ successfully finds them and generates a script to
revoke non-default ACL. Though, without
test_rename_catalog_objects.patch, /pg_upgrade --check/ still generates
the same message, which is false positive.

I am going to fix it and send the updated patch later this week.

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attachment Content-Type Size
pg_upgrade_ACL_test.sh application/x-shellscript 2.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Ibrar Ahmed 2020-03-24 17:06:47 Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits
Previous Message Robert Haas 2020-03-24 17:00:38 Re: backup manifests