Re: pg_upgrade fails with non-standard ACL

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.

> --- 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?

> + 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.

> + }
> +
> + /* 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.

> --- a/src/bin/pg_upgrade/info.c
> +++ b/src/bin/pg_upgrade/info.c

> +get_non_default_acl_infos(ClusterInfo *cluster)
> +{

> + res = executeQueryOrDie(conn,
> + /*
> + * Get relations, attributes, functions and procedures. Some system
> + * objects like views are not pinned, but these type of objects are
> + * created in pg_catalog schema.
> + */
> + "SELECT obj.type, obj.identity, shd.refobjid::regrole rolename, "
> + " CASE WHEN shd.classid = 'pg_class'::regclass THEN true "
> + " ELSE false "
> + " END is_relation "
> + "FROM pg_catalog.pg_shdepend shd, "
> + "LATERAL pg_catalog.pg_identify_object("
> + " shd.classid, shd.objid, shd.objsubid) obj "
> + /* 'a' is for SHARED_DEPENDENCY_ACL */
> + "WHERE shd.deptype = 'a' AND shd.dbid = %d "
> + " AND shd.classid IN ('pg_proc'::regclass, 'pg_class'::regclass) "
> + /* get only system objects */
> + " AND obj.schema = 'pg_catalog' "

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:

===
Checking for large objects ok
SQL command failed
SELECT obj.type, obj.identity, shd.refobjid::regrole rolename, CASE WHEN shd.classid = 'pg_class'::regclass THEN true ELSE false END is_relation FROM pg_catalog.pg_shdepend shd, LATERAL pg_catalog.pg_identify_object( shd.classid, shd.objid, shd.objsubid) obj WHERE shd.deptype = 'a' AND shd.dbid = 11564 AND shd.classid IN ('pg_proc'::regclass, 'pg_class'::regclass) AND obj.schema = 'pg_catalog' ORDER BY obj.identity COLLATE "C";
ERROR: syntax error at or near "."
LINE 1: ...ROM pg_catalog.pg_shdepend shd, LATERAL pg_catalog.pg_identi...
^
Failure, exiting
===

> + while (aclnum < PQntuples(res) &&
> + strcmp(curr->obj_ident, PQgetvalue(res, aclnum, i_obj_ident)) == 0)

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.)

nm