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