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

On Mon, Mar 14, 2016 at 7:00 PM, Stephen Frost <sfr...@snowman.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

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

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

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

Reply via email to