On Tue, Apr 05, 2016 at 05:50:18PM -0400, Stephen Frost wrote:
> I'll be doing more testing, review and clean-up (there are some
> whitespace only changes in the later patches that really should be
> included in the earlier patches where the code was actually changed)
> tonight with plans to push this tomorrow night.

(1) I ran into trouble reconciling the scope of dumpable ACLs.  The commit
message indicated this scope:

> Subject: [PATCH 4/5] In pg_dump, include pg_catalog and extension ACLs, if
>  changed
> 
> Now that all of the infrastructure exists, add in the ability to
> dump out the ACLs of the objects inside of pg_catalog or the ACLs
> for objects which are members of extensions, but only if they have
> been changed from their original values.

I wrote the attached test script to verify which types of ACLs dump/reload
covers.  Based on the commit message, I expected these results:

  Dumped: type, class, attribute, proc, largeobject_metadata,
      foreign_data_wrapper, foreign_server,
      language(in-extension), namespace(in-extension)
  Not dumped: database, tablespace,
      language(from-initdb), namespace(from-initdb)

Did I interpret the commit message correctly?  The script gave these results:

  Dumped: type, class, attribute, namespace(from-initdb)
  Not dumped: database, tablespace, proc, language(from-initdb)
  Not tested: largeobject_metadata, foreign_data_wrapper, foreign_server,
      language(in-extension), namespace(in-extension)

I didn't dig into why that happened; the choice of object type may not even be
the root cause in each case.  Which of those results, if any, surprise you?


(2) pg_dump queries:

> @@ -14187,18 +14869,65 @@ dumpTable(Archive *fout, TableInfo *tbinfo)

> +                                                       "FROM 
> pg_catalog.pg_attribute at "
> +                                        "JOIN pg_catalog.pg_class c ON 
> (at.attrelid = c.oid) "
> +                                                       "LEFT JOIN 
> pg_init_privs pip ON "

Since pg_attribute and pg_class require schema qualification here, so does
pg_init_privs.  Likewise elsewhere in the patch's pg_dump changes.


(3) pg_dumpall became much slower around the time of these commits.  On one
machine (POWER7 3.55 GHz), a pg_dumpall just after initdb slowed from 0.25s at
commit 6c268df^ to 4.0s at commit 7a54270.  On a slower machine (Opteron
1210), pg_dumpall now takes 19s against such a fresh cluster.


I doubt I'll review this patch, but those things have come to my attention.
SELECT proacl FROM pg_proc WHERE proname = 'pg_sleep';
\dT+ money
\z pg_catalog.pg_database
\z information_schema.domains
\dL+ sql
\dn+ public
\l+ template1
\db+ pg_default

REVOKE EXECUTE ON FUNCTION pg_sleep(float8) FROM PUBLIC;
REVOKE USAGE ON TYPE money FROM PUBLIC;
REVOKE SELECT ON pg_database FROM PUBLIC;
GRANT SELECT (oid) ON pg_database TO PUBLIC;
REVOKE SELECT ON information_schema.domains FROM PUBLIC;
GRANT SELECT (domain_name) ON information_schema.domains TO PUBLIC;
REVOKE USAGE ON LANGUAGE sql FROM PUBLIC;
-- TODO pg_largeobject_metadata.lomacl
REVOKE USAGE ON SCHEMA public FROM PUBLIC;
REVOKE CONNECT ON DATABASE template1 FROM PUBLIC;
GRANT CREATE ON TABLESPACE pg_default TO PUBLIC;
-- SKIP pltemplate: deprecated; no DDL for it
-- TODO pg_foreign_data_wrapper.fdwacl
-- TODO pg_foreign_server.srvacl

SELECT proacl FROM pg_proc WHERE proname = 'pg_sleep';
\dT+ money
\z pg_catalog.pg_database
\z information_schema.domains
\dL+ sql
\dn+ public
\l+ template1
\db+ pg_default

/* RESULT:
$ pg_dumpall | grep -E '(GRANT|REVOKE)'
GRANT ALL ON DATABASE template1 TO nm;
REVOKE ALL ON SCHEMA public FROM PUBLIC;
GRANT CREATE ON SCHEMA public TO PUBLIC;
REVOKE ALL ON TYPE money FROM PUBLIC;
REVOKE SELECT ON TABLE pg_database FROM PUBLIC;
GRANT SELECT(oid) ON TABLE pg_database TO PUBLIC;
*/
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to