Re: pg_upgrade fails with non-standard ACL

On Mon, Oct 28, 2019 at 05:40:44PM +0300, Anastasia Lubennikova wrote:
> I added more comments and updated the error message.
> Please, feel free to fix them, if you have any suggestions.

I have begun looking at this one.

+ /* REVOKE command must be executed in corresponding database */
+ if (*new_db)
+ {
+ fprintf(*script, _("\\c %s \n"), olddbinfo->db_name);
+ *new_db = false;
+ }
This will fail if the database to use includes a space? And it seems
to me that log_incompatible_procedure() does not quote things
properly either.

+ * from initial privilleges. Only check privileges set by initdb.
s/privilleges/privileges/

I think that there is little point to keep get_catalog_procedures()
and check_catalog_procedures() separated. Why not just using a single
entry point.

Wouldn't it be more simple to just use a cast as
pg_proc.oid::regprocedure in the queries?

Let's also put some effort in the formatting of the SQL queries here:
- Schema-qualification with pg_catalog.
- Format of the clauses could be better (for examples two-space
indentation for clauses, etc.)

I think that we should keep the core part of the fix more simple by
just warning about missing function signatures in the target cluster
when pg_upgrade --check is used. So I think that it would be better
for now to get to a point where we can warn about the function
signatures involved in a given database, without the generation of the
script with those REVOKE queries. Or would folks prefer keep that in
the first version? My take would be to handle both separately, and to
warn about everything so as there is no need to do pg_upgrade --check
more than once.

I may be missing something, but it seems to me that there is no need
to attach proc_arr to DbInfo or have traces of it in pg_upgrade.h as
long as you keep the checks of function signatures local to the single
entry point I am mentioning above.
--
Michael