Re: pg_upgrade fails with non-standard ACL

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