Re: pg_upgrade fails with non-standard ACL - Mailing list pgsql-hackers

On 03.01.2021 14:29, Noah Misch wrote:
> On Thu, Jun 11, 2020 at 07:58:43PM +0300, Anastasia Lubennikova wrote:
>> On 08.06.2020 19:31, Alvaro Herrera wrote:
>>> I'm thinking what's a good way to have a test that's committable.  Maybe
>>> it would work to add a TAP test to pg_upgrade that runs initdb, does a
>>> few GRANTS as per your attachment, then runs pg_upgrade?  Currently the
>>> pg_upgrade tests are not TAP ... we'd have to revive
>>> https://postgr.es/m/20180126080026.GI17847@paquier.xyz
>>> (Some progress was already made on the buildfarm front to permit this)
>> I would be glad to add some test, but it seems to me that the infrastructure
>> changes for cross-version pg_upgrade test is much more complicated task than
>> this modest bugfix.
> Agreed.
Thank you for the review.
New version of the patch is attached, though I haven't tested it 
properly yet. I am planning to do in a couple of days.
>> --- a/src/bin/pg_upgrade/check.c
>> +++ b/src/bin/pg_upgrade/check.c
>> +static void
>> +check_for_changed_signatures(void)
>> +{
>> +    snprintf(output_path, sizeof(output_path), "revoke_objects.sql");
> If an upgrade deleted function pg_last_wal_replay_lsn, upgrading a database in
> which "REVOKE ALL ON FUNCTION pg_last_wal_replay_lsn FROM public" happened
> requires a GRANT.  Can you use a file name that will still make sense if
> someone enhances pg_upgrade to generate such GRANT statements?
I changed the name to 'fix_system_objects_ACL.sql'. Does it look good to 
you?

>
>> +                if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
>> +                    pg_fatal("could not open file \"%s\": %s\n",
>> +                             output_path, strerror(errno));
> Use %m instead of passing sterror(errno) to %s.
Done.
>> +                }
>> +
>> +                /* Handle columns separately */
>> +                if (strstr(aclinfo->obj_type, "column") != NULL)
>> +                {
>> +                    char       *pdot = last_dot_location(aclinfo->obj_ident);
> I'd write strrchr(aclinfo->obj_ident, '.') instead of introducing
> last_dot_location().
>
> This assumes column names don't contain '.'; how difficult would it be to
> remove that assumption?  If it's difficult, a cheap approach would be to
> ignore any entry containing too many dots.  We're not likely to create such
> column names at initdb time, but v13 does allow:
>
>    ALTER VIEW pg_locks RENAME COLUMN objid TO "a.b";
>    GRANT SELECT ("a.b") ON pg_locks TO backup;
>
> After which revoke_objects.sql has:
>
>    psql:./revoke_objects.sql:9: ERROR:  syntax error at or near "") ON pg_catalog.pg_locks.""
>    LINE 1: REVOKE ALL (b") ON pg_catalog.pg_locks."a FROM "backup";
>
> While that ALTER VIEW probably should have required allow_system_table_mods,
> we need to assume such databases exist.

I've fixed it by using pg_identify_object_as_address(). Now we don't 
have to parse obj_identity.

>
> Overall, this patch predicts a subset of cases where pg_dump will emit a
> failing GRANT or REVOKE that targets a pg_catalog object.  Can you write a
> code comment stating what it does and does not detect?  I think it's okay to
> not predict every failure, but let's record the intended coverage.  Here are a
> few examples I know; there may be more.  The above query only finds GRANTs to
> non-pinned roles.  pg_dump dumps the following, but the above query doesn't
> find them:
>
>    REVOKE ALL ON FUNCTION pg_last_wal_replay_lsn FROM public;
>    GRANT EXECUTE ON FUNCTION pg_reload_conf() TO pg_signal_backend;
>
> The above query should test refclassid.

>
> This function should not run queries against servers older than 9.6, because
> pg_dump emits GRANT/REVOKE for pg_catalog objects only when targeting 9.6 or
> later.  An upgrade from 8.4 to master is failing on the above query:
>
Fixed.

> The patch adds many lines wider than 78 columns, this being one example.  In
> general, break such lines.  (Don't break string literal arguments of
> ereport(), though.)
Fixed.
> nm


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