Re: pg_dump dump catalog ACLs

Hi, Stephen!

I'm signed to review this patch. At first, patch wasn't applied cleanly, it
had a conflict at the end of system_views.sql. But that way very easy to
fix.

On Mon, Mar 14, 2016 at 7:00 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> I've not included it in this patch, but my thinking here would be to add
> a check to the GRANT functions which checks 'if (creating_extension)'
> and records/updates the entries in pg_init_privs for those objects.
> This is similar to how we handle dependencies for extensions currently.
> That way, extensions don't have to do anything and if the user changes
> the permissions on an extension's object, the permissions for that
> object would then be dumped out.
>
> This still requires more testing, documentation, and I'll see about
> making it work completely for extensions also, but wanted to provide an
> update and solicit for review/comments.
>
> The patches have been broken up in what I hope is a logical way for
> those who wish to review/comment:
>
> #1 - Add the pg_init_privs catalog
> #2 - Change pg_dump to use a bitmap instead of boolean for 'dump'
> #3 - Split 'dump' into 'dump' and 'dump_contains'
> #4 - Make pg_dump include changed ACLs in dump of 'pg_catalog'
>

+ "CASE WHEN pip.initprivs IS NOT DISTINCT FROM n.nspacl "
+ "THEN false ELSE true END as dumpacl "

Why don't just write " pip.initprivs IS DISTINCT FROM n.nspacl AS dumpacl"?
I think there are few places whene CASE could be eliminated.

+ appendPQExpBuffer(query, "SELECT tableoid, oid, proname AS aggname, "
+ "pronamespace AS aggnamespace, "
+ "pronargs, proargtypes, "
+ "(%s proowner) AS rolname, "
+ "proacl AS aggacl "
+ "FROM pg_proc p "
+ "WHERE proisagg AND ("
+ "pronamespace != "
+ "(SELECT oid FROM pg_namespace "
+ "WHERE nspname = 'pg_catalog') OR "
+ "EXISTS (SELECT * FROM pg_init_privs pip "
+ "WHERE p.oid = pip.objoid AND pip.classoid = "
+ "(SELECT oid FROM pg_class "
+ "WHERE relname = 'pg_proc') "
+ "AND p.proacl <> pip.initprivs)",
+ username_subquery);

Why do we compare ACLs using <> funcs and using IS DISTINCT for other
objects? Is it intentional? Assuming most of proacl values are NULLs when
you change it, acl wouldn't be dumped since NULL <> value IS NULL.

In general, these checks using subqueries looks complicated for me, hard to
validate and needs a lot of testing. Could we implement function "bool
need_acl_dump(oid objoid, oid classoid, int objsubid, proacl acl)" and call
it?

> #5 - Remove 'if (!superuser()) ereport()' calls and REVOKE EXECUTE
>

Other things in the patches looks good for me except they need tests and
documentation.
I'm marking this waiting for author.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company