Re: [HACKERS] pg_dump dump catalog ACLs
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > The subquery comparing the OID of pg_class using only a condition on > relname seems wrong; wouldn't it fail or produce wrong results if > somebody creates a table named pg_class in another schema? I think you > should write the comparison like this instead: > classoid = 'pg_catalog.pg_class'::regclass Thanks, patch attached which does that (qualifying to the same level as the surrounding query for each). I've run it through my tests and will plan to push it tomorrow. Thanks! Stephen From e294008daa8e909059c94441643157fddf9af9b6 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Tue, 10 May 2016 12:55:11 -0400 Subject: [PATCH] Qualify table usage in dumpTable() and use regclass All of the other tables used in the query in dumpTable(), which is collecting column-level ACLs, are qualified, so we should be qualifying the pg_init_privs and related sub-select against pg_class too. Also, use ::regclass (or ::pg_catalog.regclass, where appropriate) instead of using a poorly constructed query to get the OID for various catalog tables. Issues identified by Noah and Alvaro, patch by me. --- src/bin/pg_dump/pg_dump.c | 59 --- 1 file changed, 30 insertions(+), 29 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 1267afb..1d985e4 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -2825,8 +2825,8 @@ getBlobs(Archive *fout) "%s AS initrlomacl " "FROM pg_largeobject_metadata l " "LEFT JOIN pg_init_privs pip ON " - "(l.oid = pip.objoid AND pip.classoid = " -"(SELECT oid FROM pg_class WHERE relname = 'pg_largeobject')" + "(l.oid = pip.objoid " + "AND pip.classoid = 'pg_largeobject'::regclass " "AND pip.objsubid = 0) ", username_subquery, acl_subquery->data, @@ -3569,8 +3569,8 @@ getNamespaces(Archive *fout, int *numNamespaces) "%s as initrnspacl " "FROM pg_namespace n " "LEFT JOIN pg_init_privs pip " - "ON (n.oid = pip.objoid AND pip.classoid = " - "(SELECT oid FROM pg_class WHERE relname = 'pg_namespace') " + "ON (n.oid = pip.objoid " + "AND pip.classoid = 'pg_namespace'::regclass " "AND pip.objsubid = 0) ", username_subquery, acl_subquery->data, @@ -3851,9 +3851,9 @@ getTypes(Archive *fout, int *numTypes) "t.typname[0] = '_' AND t.typelem != 0 AND " "(SELECT typarray FROM pg_type te WHERE oid = t.typelem) = t.oid AS isarray " "FROM pg_type t " - "LEFT JOIN pg_init_privs pip " - "ON (t.oid = pip.objoid AND pip.classoid = " - "(SELECT oid FROM pg_class WHERE relname = 'pg_type') " + "LEFT JOIN pg_init_privs pip ON " + "(t.oid = pip.objoid " + "AND pip.classoid = 'pg_type'::regclass " "AND pip.objsubid = 0) ", acl_subquery->data, racl_subquery->data, @@ -4713,8 +4713,8 @@ getAggregates(Archive *fout, int *numAggs) "%s AS initraggacl " "FROM pg_proc p " "LEFT JOIN pg_init_privs pip ON " - "(p.oid = pip.objoid AND pip.classoid = " - "(SELECT oid FROM pg_class WHERE relname = 'pg_proc') " + "(p.oid = pip.objoid " + "AND pip.classoid = 'pg_proc'::regclass " "AND pip.objsubid = 0) " "WHERE p.proisagg AND (" "p.pronamespace != " @@ -4956,8 +4956,8 @@ getFuncs(Archive *fout, int *numFuncs) "(%s p.proowner) AS rolname " "FROM pg_proc p " "LEFT JOIN pg_init_privs pip ON " - "(p.oid = pip.objoid AND pip.classoid = " - "(SELECT oid FROM pg_class WHERE relname = 'pg_proc') " + "(p.oid = pip.objoid " + "AND pip.classoid = 'pg_proc'::regclass " "AND pip.objsubid = 0) " "WHERE NOT proisagg " "AND NOT EXISTS (SELECT 1 FROM pg_depend " @@ -5246,9 +5246,10 @@ getTables(Archive *fout, int *numTables) "CASE WHEN 'check_option=local' = ANY (c.reloptions) THEN 'LOCAL'::text " "WHEN 'check_option=cascaded' = ANY (c.reloptions) THEN 'CASCADED'::text ELSE NULL END AS checkoption, " "tc.reloptions AS toast_reloptions, " - "EXISTS (SELECT 1 FROM pg_attribute at LEFT JOIN pg_init_privs pip ON" - "(c.oid = pip.objoid AND pip.classoid = " - "(SELECT oid FROM pg_class WHERE relname = 'pg_class') AND pip.objsubid = at.attnum)" + "EXISTS (SELECT 1 FROM pg_attribute at LEFT JOIN pg_init_privs pip ON " + "(c.oid = pip.objoid " + "AND pip.classoid = 'pg_class'::regclass " + "AND pip.objsubid = at.attnum)" "WHERE at.attrelid = c.oid AND (" "%s IS NOT NULL " "OR %s IS NOT NULL " @@ -5264,9 +5265,9 @@ getTables(Archive *fout, int *numTables) "d.refclassid = c.tableoid AND d.deptype = 'a') " "LEFT JOIN pg_class tc ON (c.reltoastrelid = tc.oid) " "LEFT JO
Re: [HACKERS] pg_dump dump catalog ACLs
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Stephen Frost wrote: > > > diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c > > index 1267afb..4a9b1bf 100644 > > --- a/src/bin/pg_dump/pg_dump.c > > +++ b/src/bin/pg_dump/pg_dump.c > > @@ -14992,9 +14992,10 @@ dumpTable(Archive *fout, TableInfo *tbinfo) > > "%s AS initrattacl " > > "FROM > > pg_catalog.pg_attribute at " > >"JOIN pg_catalog.pg_class c ON > > (at.attrelid = c.oid) " > > - "LEFT JOIN > > pg_init_privs pip ON " > > + "LEFT JOIN > > pg_catalog.pg_init_privs pip ON " > > "(pip.classoid = " > > -"(SELECT oid FROM pg_class WHERE relname = > > 'pg_class') AND " > > + "(SELECT oid FROM > > pg_catalog.pg_class " > > + "WHERE relname = > > 'pg_class') AND " > >" at.attrelid = pip.objoid AND at.attnum = > > pip.objsubid) " > > "WHERE at.attrelid = > > '%u' AND " > > "NOT at.attisdropped " > > The subquery comparing the OID of pg_class using only a condition on > relname seems wrong; wouldn't it fail or produce wrong results if > somebody creates a table named pg_class in another schema? I think you > should write the comparison like this instead: > classoid = 'pg_catalog.pg_class'::regclass Errr, I could have sworn I was doing that, but clearly I'm not. Will fix. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] pg_dump dump catalog ACLs
Stephen Frost wrote: > diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c > index 1267afb..4a9b1bf 100644 > --- a/src/bin/pg_dump/pg_dump.c > +++ b/src/bin/pg_dump/pg_dump.c > @@ -14992,9 +14992,10 @@ dumpTable(Archive *fout, TableInfo *tbinfo) > "%s AS initrattacl " > "FROM > pg_catalog.pg_attribute at " > "JOIN pg_catalog.pg_class c ON > (at.attrelid = c.oid) " > - "LEFT JOIN > pg_init_privs pip ON " > + "LEFT JOIN > pg_catalog.pg_init_privs pip ON " > "(pip.classoid = " > - "(SELECT oid FROM pg_class WHERE relname = > 'pg_class') AND " > + "(SELECT oid FROM > pg_catalog.pg_class " > + "WHERE relname = > 'pg_class') AND " > " at.attrelid = pip.objoid AND at.attnum = > pip.objsubid) " > "WHERE at.attrelid = > '%u' AND " > "NOT at.attisdropped " The subquery comparing the OID of pg_class using only a condition on relname seems wrong; wouldn't it fail or produce wrong results if somebody creates a table named pg_class in another schema? I think you should write the comparison like this instead: classoid = 'pg_catalog.pg_class'::regclass -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump dump catalog ACLs
* Noah Misch (n...@leadboat.com) wrote: > On Tue, Apr 05, 2016 at 05:50:18PM -0400, Stephen Frost wrote: > > @@ -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. Attached is a patch to qualify the tables used in that query. I reviewed all of the other pg_init_privs uses and they all match the qualification level of the other tables in those queries. This isn't too surprising as this is the only query which happens outside of the "get*()" functions. This patch doesn't do anything but qualify the table usage and that query is checked by the pg_dump regression suite and ran fine for me, so, if there are no objections, I'll push this later on this afternoon. Thanks! Stephen From b8032e96ad1865a824ea2feb2f2a34f9b29ac6ac Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Tue, 10 May 2016 12:55:11 -0400 Subject: [PATCH] Qualify table usage in dumpTable() All of the other tables used in the query in dumpTable(), which is collecting column-level ACLs, are qualified, so we should be qualifying the pg_init_privs and related sub-select against pg_class too. Pointed out by Noah, patch by me. --- src/bin/pg_dump/pg_dump.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 1267afb..4a9b1bf 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -14992,9 +14992,10 @@ dumpTable(Archive *fout, TableInfo *tbinfo) "%s AS initrattacl " "FROM pg_catalog.pg_attribute at " "JOIN pg_catalog.pg_class c ON (at.attrelid = c.oid) " - "LEFT JOIN pg_init_privs pip ON " + "LEFT JOIN pg_catalog.pg_init_privs pip ON " "(pip.classoid = " - "(SELECT oid FROM pg_class WHERE relname = 'pg_class') AND " + "(SELECT oid FROM pg_catalog.pg_class " + "WHERE relname = 'pg_class') AND " " at.attrelid = pip.objoid AND at.attnum = pip.objsubid) " "WHERE at.attrelid = '%u' AND " "NOT at.attisdropped " -- 2.5.0 signature.asc Description: Digital signature
Re: [HACKERS] pg_dump dump catalog ACLs
* Stephen Frost (sfr...@snowman.net) wrote: > In any case, as I was saying, that's far closer to 9.5 run-time. I've > not measured the time added when things like TRANSFORMs were added, but > it wouldn't surprise me if adding a new query for every database to > pg_dump adds something similar to this difference. Updated patch attached. This adds the same kind of test suite to a new 'src/test/modules/test_pg_dump' for testing pg_dump with extensions. I'm going to flesh that out tomorrow with some real tests. Today was spent mostly setting up that test suite and spending quite a bit of time cleaning up the src/bin/pg_dump/t tests, to make it easier for others to review and make sense of the regexp's and whatnot that are included. Also updated the skipping of LOCK TABLE, per comments from Tom. I should be able to get the testing that I want added to test_pg_dump tomorrow and will push all of this once that's included. I'd really like to make sure that we've got coverage of initial privileges in extensions with pg_dump, which is why I've been working on this today and haven't pushed the patch-set yet. I'm pretty sure they work as expected, but we should be testing it through the buildfarm. Thanks! Stephen From 1e3d3053d9cbb309af0307e31ddea92999d9ed15 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Sun, 24 Apr 2016 23:59:23 -0400 Subject: [PATCH 1/4] Correct pg_dump WHERE clause for functions/aggregates The query to grab the function/aggregate information is now joining to pg_init_privs, so we can simplify (and correct) the WHERE clause used to determine if a given function's ACL has changed from the initial ACL on the function. Bug found by Noah, patch by me. --- src/bin/pg_dump/pg_dump.c | 12 ++-- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index d3f5157..bb33075 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -4673,11 +4673,7 @@ getAggregates(Archive *fout, int *numAggs) "p.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 IS DISTINCT FROM pip.initprivs)", + "p.proacl IS DISTINCT FROM pip.initprivs", username_subquery, acl_subquery->data, racl_subquery->data, @@ -4923,11 +4919,7 @@ getFuncs(Archive *fout, int *numFuncs) "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 IS DISTINCT FROM pip.initprivs)", + "p.proacl IS DISTINCT FROM pip.initprivs", acl_subquery->data, racl_subquery->data, initacl_subquery->data, -- 2.5.0 From 374d17371aff04f3887fa980337154b5c2e8db22 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Mon, 25 Apr 2016 00:00:15 -0400 Subject: [PATCH 2/4] pg_dump performance and other fixes Do not try to dump objects which do not have ACLs when only ACLs are being requested. This results in a significant performance improvement as we can avoid querying for further information on these objects when we don't need to. When limiting the components to dump for an extension, consider what components have been requested. Initially, we incorrectly hard-coded the components of the extension objects to dump, which would mean that we wouldn't dump some components even with they were asked for and in other cases we would dump components which weren't requested. Correct defaultACLs to use 'dump_contains' instead of 'dump'. The defaultACL is considered a member of the namespace and should be dumped based on the same set of components that the other objects in the schema are, not based on what we're dumping for the namespace itself (which might not include ACLs, if the namespace has just the default or initial ACL). Use DUMP_COMPONENT_ACL for from-initdb objects, to allow users to change their ACLs, should they wish to. This just extends what we are doing for the pg_catalog namespace to objects which are not members of namespaces. --- src/bin/pg_dump/pg_dump.c | 176 +++--- 1 file changed, 149 insertions(+), 27 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index bb33075..d826b4d 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -1297,14 +1297,17 @@ checkExtensionMembership(DumpableObject *dobj, Archive *fout) * contents rather than replace the extension contents with something * different. */ - if (!fout->dopt->binary_upgrade && fout->remoteVersion >= 90600) - dobj->dum
Re: [HACKERS] pg_dump dump catalog ACLs
* Noah Misch (n...@leadboat.com) wrote: > On Wed, May 04, 2016 at 08:14:55AM -0400, Stephen Frost wrote: > > * Noah Misch (n...@leadboat.com) wrote: > > > On Mon, Apr 25, 2016 at 12:39:09AM -0400, Stephen Frost wrote: > > > > * Noah Misch (n...@leadboat.com) wrote: > > > > > On Fri, Apr 22, 2016 at 12:31:41PM -0400, Stephen Frost wrote: > > > > > > After looking through the code a bit, I realized that there are a > > > > > > lot of > > > > > > object types which don't have ACLs at all but which exist in > > > > > > pg_catalog > > > > > > and were being analyzed because the bitmask for pg_catalog included > > > > > > ACLs > > > > > > and therefore was non-zero. > > > > > > > > > > > > Clearing that bit for object types which don't have ACLs improved > > > > > > the > > > > > > performance for empty databases quite a bit (from about 3s to a bit > > > > > > under 1s on my laptop). That's a 42-line patch, with comment lines > > > > > > being half of that, which I'll push once I've looked into the other > > > > > > concerns which were brought up on this thread. > > > > > > > > > > That's good news. > > > > > > > > Attached patch-set includes this change in patch #2. > > > > > > Timings for the 100-database pg_dumpall: > > > > > > HEAD: 131s > > > HEAD+patch: 33s > > > 9.5:8.6s > > > > > > Nice improvement for such a simple patch. > > > > Patch #2 in the attached patchset includes that improvement and a > > further one which returns the performance to very close to 9.5. > > What timings did you measure? (How close?) On my laptop, with 100 databases, I get: 9.5: 0m3.306s, 0m3.290s, 0m3.309s, avg: 3.302 HEAD+patch: 0m4.681s, 0m4.681s, 0m4.709s, avg: 4.690 9.5 was installed from Debian packages, HEAD+patch was built with -O2, no debugging, no casserts, though it's entirely possible I've got something else enabled in my builds that slow it down a bit. In any case, as I was saying, that's far closer to 9.5 run-time. I've not measured the time added when things like TRANSFORMs were added, but it wouldn't surprise me if adding a new query for every database to pg_dump adds something similar to this difference. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] pg_dump dump catalog ACLs
On Wed, May 04, 2016 at 08:14:55AM -0400, Stephen Frost wrote: > * Noah Misch (n...@leadboat.com) wrote: > > On Mon, Apr 25, 2016 at 12:39:09AM -0400, Stephen Frost wrote: > > > * Noah Misch (n...@leadboat.com) wrote: > > > > On Fri, Apr 22, 2016 at 12:31:41PM -0400, Stephen Frost wrote: > > > > > After looking through the code a bit, I realized that there are a lot > > > > > of > > > > > object types which don't have ACLs at all but which exist in > > > > > pg_catalog > > > > > and were being analyzed because the bitmask for pg_catalog included > > > > > ACLs > > > > > and therefore was non-zero. > > > > > > > > > > Clearing that bit for object types which don't have ACLs improved the > > > > > performance for empty databases quite a bit (from about 3s to a bit > > > > > under 1s on my laptop). That's a 42-line patch, with comment lines > > > > > being half of that, which I'll push once I've looked into the other > > > > > concerns which were brought up on this thread. > > > > > > > > That's good news. > > > > > > Attached patch-set includes this change in patch #2. > > > > Timings for the 100-database pg_dumpall: > > > > HEAD: 131s > > HEAD+patch: 33s > > 9.5: 8.6s > > > > Nice improvement for such a simple patch. > > Patch #2 in the attached patchset includes that improvement and a > further one which returns the performance to very close to 9.5. What timings did you measure? (How close?) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump dump catalog ACLs
Noah, all, * Stephen Frost (sfr...@snowman.net) wrote: > The test suite is now covering 57% of pg_dump.c. I was hoping to get > that number higher, but time marches on and more tests can certainly be > added later. I've managed to get the test suite up another 10%, to 67% of pg_dump.c. Still quite a bit to do, but it turned up a bug in CREATE TRANSFORM, which lead to Peter discovering a similar issue in CREATE CAST, so I'd say it's certainly showing that it's worthwhile to have. Updated patch-set which includes the additional tests and also a patch to avoid LOCK'ing tables when we don't need to, as discussed with Tom and Robert on another thread. Thanks! Stephen From cea33c3fdbdfa4ff27c0ff6ed7b708705dfa17fe Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Sun, 24 Apr 2016 23:59:23 -0400 Subject: [PATCH 1/4] Correct pg_dump WHERE clause for functions/aggregates The query to grab the function/aggregate information is now joining to pg_init_privs, so we can simplify (and correct) the WHERE clause used to determine if a given function's ACL has changed from the initial ACL on the function. Bug found by Noah, patch by me. --- src/bin/pg_dump/pg_dump.c | 12 ++-- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index d3f5157..bb33075 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -4673,11 +4673,7 @@ getAggregates(Archive *fout, int *numAggs) "p.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 IS DISTINCT FROM pip.initprivs)", + "p.proacl IS DISTINCT FROM pip.initprivs", username_subquery, acl_subquery->data, racl_subquery->data, @@ -4923,11 +4919,7 @@ getFuncs(Archive *fout, int *numFuncs) "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 IS DISTINCT FROM pip.initprivs)", + "p.proacl IS DISTINCT FROM pip.initprivs", acl_subquery->data, racl_subquery->data, initacl_subquery->data, -- 2.5.0 From 707707ba1c2ee601ce893258eaaf5c54341806f6 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Mon, 25 Apr 2016 00:00:15 -0400 Subject: [PATCH 2/4] pg_dump performance and other fixes Do not try to dump objects which do not have ACLs when only ACLs are being requested. This results in a significant performance improvement as we can avoid querying for further information on these objects when we don't need to. When limiting the components to dump for an extension, consider what components have been requested. Initially, we incorrectly hard-coded the components of the extension objects to dump, which would mean that we wouldn't dump some components even with they were asked for and in other cases we would dump components which weren't requested. Correct defaultACLs to use 'dump_contains' instead of 'dump'. The defaultACL is considered a member of the namespace and should be dumped based on the same set of components that the other objects in the schema are, not based on what we're dumping for the namespace itself (which might not include ACLs, if the namespace has just the default or initial ACL). Use DUMP_COMPONENT_ACL for from-initdb objects, to allow users to change their ACLs, should they wish to. This just extends what we are doing for the pg_catalog namespace to objects which are not members of namespaces. --- src/bin/pg_dump/pg_dump.c | 176 +++--- 1 file changed, 149 insertions(+), 27 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index bb33075..d826b4d 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -1297,14 +1297,17 @@ checkExtensionMembership(DumpableObject *dobj, Archive *fout) * contents rather than replace the extension contents with something * different. */ - if (!fout->dopt->binary_upgrade && fout->remoteVersion >= 90600) - dobj->dump = DUMP_COMPONENT_ACL | - DUMP_COMPONENT_SECLABEL | - DUMP_COMPONENT_POLICY; - else if (!fout->dopt->binary_upgrade) - dobj->dump = DUMP_COMPONENT_NONE; - else + if (fout->dopt->binary_upgrade) dobj->dump = ext->dobj.dump; + else + { + if (fout->remoteVersion < 90600) + dobj->dump = DUMP_COMPONENT_NONE; + else + dobj->dump = ext->dobj.dump_contains & (DUMP_COMPONENT_ACL | + DUMP_COMPONENT_SECLABEL | DUMP_COMPONENT_POLICY); + + } return true; } @@ -1452,7 +1455,7 @@ selectDumpableDefaultACL(DefaultACLInfo *dinfo, DumpOptions
Re: [HACKERS] pg_dump dump catalog ACLs
Noah, * Noah Misch (n...@leadboat.com) wrote: > On Mon, Apr 25, 2016 at 12:39:09AM -0400, Stephen Frost wrote: > > * Noah Misch (n...@leadboat.com) wrote: > > > On Fri, Apr 22, 2016 at 12:31:41PM -0400, Stephen Frost wrote: > > > > After looking through the code a bit, I realized that there are a lot of > > > > object types which don't have ACLs at all but which exist in pg_catalog > > > > and were being analyzed because the bitmask for pg_catalog included ACLs > > > > and therefore was non-zero. > > > > > > > > Clearing that bit for object types which don't have ACLs improved the > > > > performance for empty databases quite a bit (from about 3s to a bit > > > > under 1s on my laptop). That's a 42-line patch, with comment lines > > > > being half of that, which I'll push once I've looked into the other > > > > concerns which were brought up on this thread. > > > > > > That's good news. > > > > Attached patch-set includes this change in patch #2. > > Timings for the 100-database pg_dumpall: > > HEAD: 131s > HEAD+patch: 33s > 9.5:8.6s > > Nice improvement for such a simple patch. Patch #2 in the attached patchset includes that improvement and a further one which returns the performance to very close to 9.5. This is done by checking for any changed ACLs for a given table (the table-level and column-level ones) and removing the ACL component if there haven't been any changes. > > Patch #1 is the fix for the incorrect WHERE clause. > > I confirmed that this fixed dumping of the function ACL in my test case. This patch is unchanged. > > For languages, I believe that means that we simply need to modify the > > selectDumpableProcLang() function to use the same default we use for the > > 'pg_catalog' namespace- DUMP_COMPONENT_ACL, instead of > > DUMP_COMPONENT_NONE. > > Makes sense. This is now being tested in the TAP testing for pg_dump that I've been working on (patch #3). Fixing this issue was a bit more complicated because of cases like plpgsql, which is a from-initdb extension, but we still want to dump out any changes to plpgsql that the user has made, so we have to dump ACLs for from-initdb extension objects, if they've been changed. Those fixes are included in patch #2 in the patchset. > > What's not clear to me is what, if any, issue there is with namespaces. > > As far as I know, none. The current behavior doesn't match the commit > message, but I think the current behavior is better. Great. No changes have been made to how namespaces are handled. > > Certainly, in my testing at least, if you do: > > > > REVOKE CREATE ON SCHEMA public FROM public; > > > > Then you get the appropriate commands from pg_dump to implement the > > resulting ACLs on the public schema. If you change the permissions back > > to match what is there at initdb-time (or you just don't change them), > > then there aren't any GRANT or REVOKE commands from pg_dump, but that's > > correct, isn't it? > > Good question. I think it's fine and possibly even optimal. One can imagine > other designs that remember whether any GRANT or REVOKE has happened since > initdb, but I see no definite reason to prefer that alternative. Neither do I. > > In the attached patch-set, patch #3 includes support for > > > > src/bin/pg_dump: make check > > > > implemented using the TAP testing system. There are a total of 360 > > tests, generally covering: > > I like the structure of this test suite. Great. I've added a whole ton more tests, added more options to various pg_dump runs to test more options with fewer runs (though there are still quite a few...) and added more objects to be created. Also added a bunch of comments to describe how the test suite is set up. > > +# Start with 1 because of non-existant database test below > > +# Test connecting to a non-existant database > > Spelling. Fixed. The test suite is now covering 57% of pg_dump.c. I was hoping to get that number higher, but time marches on and more tests can certainly be added later. I'm planning to do another review of this patch-set and do testing against back-branches back to 7.4. Barring any issues or objections, I'll commit this tomorrow to address the performance concerns and to add in the test suite. On my system, the test suite takes about 15 seconds to run, which includes setting up the cluster, creating all the objects, and then running the pg_dump runs and checking the results. All told, in those 15 seconds, 1,213 tests are run. Thanks! Stephen From d8eebb233187bb4fa46c5e90d75db680d991f801 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Sun, 24 Apr 2016 23:59:23 -0400 Subject: [PATCH 1/3] Correct pg_dump WHERE clause for functions/aggregates --- src/bin/pg_dump/pg_dump.c | 12 ++-- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index d3f5157..bb33075 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -4673,11 +4673,
Re: [HACKERS] pg_dump dump catalog ACLs
On Fri, Apr 22, 2016 at 3:30 AM, Peter Geoghegan wrote: > On Fri, Apr 22, 2016 at 12:25 AM, Noah Misch wrote: >> Folks run clusters with ~1000 databases; we previously accepted at least one >> complex performance improvement[1] based on that use case. On the faster of >> the two machines I tested, the present thread's commits slowed "pg_dumpall >> --schema-only --binary-upgrade" by 1-2s per database. That doubles pg_dump >> runtime against the installcheck regression database. A run against a >> cluster >> of one hundred empty databases slowed fifteen-fold, from 8.6s to 131s. >> "pg_upgrade -j50" probably will keep things tolerable for the 1000-database >> case, but the performance regression remains jarring. I think we should not >> release 9.6 with pg_dump performance as it stands today. > > As someone that is responsible for many such clusters, I strongly agree. Stephen: This is a CRITICAL ISSUE. Unless I'm missing something, this hasn't gone anywhere in well over a week, and we're wrapping beta next Monday. Please fix it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump dump catalog ACLs
Alvaro, On Wednesday, April 27, 2016, Alvaro Herrera wrote: > Stephen Frost wrote: > > > The current challenge I've been trying to find a solution to is testing > the > > extension handling. Only core extensions (plpgsql, plperl, etc) are > > available when the TAP tests are running for pg_dump, it seems. I > certainly > > don't want to add a testing extension in such a way that it would get > > installed for users, but it doesn't seem obvious how to create an > extension > > from another directory or to even find the directory where I could write > > the control and SQL files into to use. > > > > Any thoughts on that would certainly be welcome. > > Did you notice the src/test/modules/test_extensions thingy? Supposedly > we do try pg_dump with custom extensions there. > Hmm. I think the issue I was having is that I wanted to test the extension handling in pg_dump's 'make check' run, but that's before any of those extension pieces are installed or tested. However, perhaps I can test the pg_dump parts as part of the extension 'make check' runs instead. Will check it out. Thanks! Stephen
Re: [HACKERS] pg_dump dump catalog ACLs
Stephen Frost wrote: > The current challenge I've been trying to find a solution to is testing the > extension handling. Only core extensions (plpgsql, plperl, etc) are > available when the TAP tests are running for pg_dump, it seems. I certainly > don't want to add a testing extension in such a way that it would get > installed for users, but it doesn't seem obvious how to create an extension > from another directory or to even find the directory where I could write > the control and SQL files into to use. > > Any thoughts on that would certainly be welcome. Did you notice the src/test/modules/test_extensions thingy? Supposedly we do try pg_dump with custom extensions there. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump dump catalog ACLs
Noah, On Wednesday, April 27, 2016, Noah Misch wrote: > A later thought: > > On Mon, Apr 25, 2016 at 12:39:09AM -0400, Stephen Frost wrote: > > src/bin/pg_dump: make check > > > > implemented using the TAP testing system. There are a total of 360 > > tests, generally covering: > > > > Various invalid sets of command-line options. > > > > Valid command-line options (generally independently used): > > > > (no options- defaults) > > --clean > > --clean --if-exists > > --data-only > > --format=c (tested with pg_restore) > > --format=d (tested with pg_restore) > > --format=t (tested with pg_restore) > > --format=d --jobs=2 (tested with pg_restore) > > --exclude-schema > > --exclude-table > > --no-privileges > > --no-owner > > --schema > > --schema-only > > > > Note that this is only including tests for basic schemas, tables, data, > > and privileges, so far. I believe it's pretty obvious that we want to > > include all object types and include testing of extensions, I just > > haven't gotten there yet and figured now would be a good time to solicit > > feedback on the approach I've used. > > > > I'm not sure how valuable it is to test all the different combinations > > of command-line options, though clearly some should be tested (eg: > > include-schema, exclude table in that schema, and similar cases). > > You mention that you test valid options independently. Keep an eye out for > good opportunities to save runtime by testing multiple options per > invocation. > To give one example, --exclude-table seems fairly independent of --format; > maybe those could test as a group. That complicates the suite, but saving > ten > or more seconds might justify the complexity. > That thought had occurred to me also and I began combining some options, though I could probably do more to reduce the runtime: The current challenge I've been trying to find a solution to is testing the extension handling. Only core extensions (plpgsql, plperl, etc) are available when the TAP tests are running for pg_dump, it seems. I certainly don't want to add a testing extension in such a way that it would get installed for users, but it doesn't seem obvious how to create an extension from another directory or to even find the directory where I could write the control and SQL files into to use. Any thoughts on that would certainly be welcome. Thanks! Stephen
Re: [HACKERS] pg_dump dump catalog ACLs
A later thought: On Mon, Apr 25, 2016 at 12:39:09AM -0400, Stephen Frost wrote: > src/bin/pg_dump: make check > > implemented using the TAP testing system. There are a total of 360 > tests, generally covering: > > Various invalid sets of command-line options. > > Valid command-line options (generally independently used): > > (no options- defaults) > --clean > --clean --if-exists > --data-only > --format=c (tested with pg_restore) > --format=d (tested with pg_restore) > --format=t (tested with pg_restore) > --format=d --jobs=2 (tested with pg_restore) > --exclude-schema > --exclude-table > --no-privileges > --no-owner > --schema > --schema-only > > Note that this is only including tests for basic schemas, tables, data, > and privileges, so far. I believe it's pretty obvious that we want to > include all object types and include testing of extensions, I just > haven't gotten there yet and figured now would be a good time to solicit > feedback on the approach I've used. > > I'm not sure how valuable it is to test all the different combinations > of command-line options, though clearly some should be tested (eg: > include-schema, exclude table in that schema, and similar cases). You mention that you test valid options independently. Keep an eye out for good opportunities to save runtime by testing multiple options per invocation. To give one example, --exclude-table seems fairly independent of --format; maybe those could test as a group. That complicates the suite, but saving ten or more seconds might justify the complexity. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump dump catalog ACLs
On Mon, Apr 25, 2016 at 12:39:09AM -0400, Stephen Frost wrote: > * Noah Misch (n...@leadboat.com) wrote: > > On Fri, Apr 22, 2016 at 12:31:41PM -0400, Stephen Frost wrote: > > > After looking through the code a bit, I realized that there are a lot of > > > object types which don't have ACLs at all but which exist in pg_catalog > > > and were being analyzed because the bitmask for pg_catalog included ACLs > > > and therefore was non-zero. > > > > > > Clearing that bit for object types which don't have ACLs improved the > > > performance for empty databases quite a bit (from about 3s to a bit > > > under 1s on my laptop). That's a 42-line patch, with comment lines > > > being half of that, which I'll push once I've looked into the other > > > concerns which were brought up on this thread. > > > > That's good news. > > Attached patch-set includes this change in patch #2. Timings for the 100-database pg_dumpall: HEAD: 131s HEAD+patch: 33s 9.5: 8.6s Nice improvement for such a simple patch. > Patch #1 is the fix for the incorrect WHERE clause. I confirmed that this fixed dumping of the function ACL in my test case. > For languages, I believe that means that we simply need to modify the > selectDumpableProcLang() function to use the same default we use for the > 'pg_catalog' namespace- DUMP_COMPONENT_ACL, instead of > DUMP_COMPONENT_NONE. Makes sense. > What's not clear to me is what, if any, issue there is with namespaces. As far as I know, none. The current behavior doesn't match the commit message, but I think the current behavior is better. > Certainly, in my testing at least, if you do: > > REVOKE CREATE ON SCHEMA public FROM public; > > Then you get the appropriate commands from pg_dump to implement the > resulting ACLs on the public schema. If you change the permissions back > to match what is there at initdb-time (or you just don't change them), > then there aren't any GRANT or REVOKE commands from pg_dump, but that's > correct, isn't it? Good question. I think it's fine and possibly even optimal. One can imagine other designs that remember whether any GRANT or REVOKE has happened since initdb, but I see no definite reason to prefer that alternative. > In the attached patch-set, patch #3 includes support for > > src/bin/pg_dump: make check > > implemented using the TAP testing system. There are a total of 360 > tests, generally covering: I like the structure of this test suite. > +# Start with 1 because of non-existant database test below > +# Test connecting to a non-existant database Spelling. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump dump catalog ACLs
Noah, all, * Noah Misch (n...@leadboat.com) wrote: > On Fri, Apr 22, 2016 at 12:31:41PM -0400, Stephen Frost wrote: > > After looking through the code a bit, I realized that there are a lot of > > object types which don't have ACLs at all but which exist in pg_catalog > > and were being analyzed because the bitmask for pg_catalog included ACLs > > and therefore was non-zero. > > > > Clearing that bit for object types which don't have ACLs improved the > > performance for empty databases quite a bit (from about 3s to a bit > > under 1s on my laptop). That's a 42-line patch, with comment lines > > being half of that, which I'll push once I've looked into the other > > concerns which were brought up on this thread. > > That's good news. Attached patch-set includes this change in patch #2. Patch #1 is the fix for the incorrect WHERE clause. > > Much of the remaining inefficiancy is how we query for column > > information one table at a time (that appears to be around 300ms of the > > 900ms or so total time). I'm certainly interested in improving that but > > that would require adding more complex data structures to pg_dump than > > what we use currently (we'd want to grab all of the columns we care > > about in an entire schema and store it locally and then provide a way to > > look up that information, etc), so I'm not sure it'd be appropriate to > > do now. > > I'm not sure, either; I'd need to see more to decide. If I were you, I would > draft a patch to the point where the community can see the complexity and the > performance change. That should be enough to inform the choice among moving > forward with the proposed complexity, soliciting other designs, reverting the > original changes, or accepting for 9.6 the slowdown as it stands at that time. > Other people may have more definite opinions already. I'll look at doing this once I'm done with the regression test improvements (see below). * Noah Misch (n...@leadboat.com) wrote: > On Fri, Apr 22, 2016 at 08:24:53PM -0400, Stephen Frost wrote: > > * Noah Misch (n...@leadboat.com) wrote: > > > 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) > > > > You interpreted the commit message correctly and in a number of cases > > the correct results are generated, but there's an issue in the WHERE > > clause being used for some of the object types. > > I'm wondering whether it would be a slightly better design to treat > language(from-initdb) like language(in-extension). Likewise for namespaces. > The code apparently already exists to dump from-initdb namespace ACLs. Is > there a user interface design reason to want to distinguish the from-initdb > and in-extension cases? Reviewing the code, we do record from-initdb namespace privileges, and we also record in-extension namespace privileges (see ExecGrant_Namespace()). We also record from-initdb language ACLs (initdb.c:2071) and in-extension language ACLs (see ExecGrant_Language()), so I'm not entirely sure what, if any, issue exists here either. For both, we also grab the ACL info vs. pg_init_privs in pg_dump. The issue in these cases is a bit of an interesting one- they are not part of a namespace; this patch was focused on allowing users to modify, specifically, the ACLs of objects in the 'pg_catalog' namespace. For languages, I believe that means that we simply need to modify the selectDumpableProcLang() function to use the same default we use for the 'pg_catalog' namespace- DUMP_COMPONENT_ACL, instead of DUMP_COMPONENT_NONE. What's not clear to me is what, if any, issue there is with namespaces. Certainly, in my testing at least, if you do: REVOKE CREATE ON SCHEMA public FROM public; Then you get the appropriate commands from pg_dump to implement the resulting ACLs on the public schema. If you change the permissions back to match what is there at initdb-time (or you just don't change them), then there aren't any GRANT or REVOKE commands from pg_dump, but that's correct, isn't it? > > That's relatively straight-forward to fix, but I'm going to put > > together a series of TAP tests to go with these fixes. While I tested > > various options and bits and pieces of the code while developing, this > > really needs a proper test suite that runs through all of these > > permutations with each change. > > Sounds great. Thanks. > > > I'm planning to have that done by the end of the
Re: [HACKERS] pg_dump dump catalog ACLs
On Fri, Apr 22, 2016 at 08:24:53PM -0400, Stephen Frost wrote: > * Noah Misch (n...@leadboat.com) wrote: > > On Tue, Apr 05, 2016 at 05:50:18PM -0400, Stephen Frost wrote: > > > 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) > > You interpreted the commit message correctly and in a number of cases > the correct results are generated, but there's an issue in the WHERE > clause being used for some of the object types. I'm wondering whether it would be a slightly better design to treat language(from-initdb) like language(in-extension). Likewise for namespaces. The code apparently already exists to dump from-initdb namespace ACLs. Is there a user interface design reason to want to distinguish the from-initdb and in-extension cases? > That's relatively straight-forward to fix, but I'm going to put > together a series of TAP tests to go with these fixes. While I tested > various options and bits and pieces of the code while developing, this > really needs a proper test suite that runs through all of these > permutations with each change. Sounds great. Thanks. > I'm planning to have that done by the end of the weekend. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump dump catalog ACLs
On Fri, Apr 22, 2016 at 12:31:41PM -0400, Stephen Frost wrote: > * Noah Misch (n...@leadboat.com) wrote: > > On Wed, Apr 20, 2016 at 10:50:21PM -0400, Stephen Frost wrote: > > > I'm certainly open to improving these issues now if we agree that they > > > should be fixed for 9.6. If we don't want to include such changes in 9.6 > > > then I will propose then for post-9.6. > > > > Folks run clusters with ~1000 databases; we previously accepted at least one > > complex performance improvement[1] based on that use case. On the faster of > > the two machines I tested, the present thread's commits slowed "pg_dumpall > > --schema-only --binary-upgrade" by 1-2s per database. That doubles pg_dump > > runtime against the installcheck regression database. A run against a > > cluster > > of one hundred empty databases slowed fifteen-fold, from 8.6s to 131s. > > "pg_upgrade -j50" probably will keep things tolerable for the 1000-database > > case, but the performance regression remains jarring. I think we should not > > release 9.6 with pg_dump performance as it stands today. > > After looking through the code a bit, I realized that there are a lot of > object types which don't have ACLs at all but which exist in pg_catalog > and were being analyzed because the bitmask for pg_catalog included ACLs > and therefore was non-zero. > > Clearing that bit for object types which don't have ACLs improved the > performance for empty databases quite a bit (from about 3s to a bit > under 1s on my laptop). That's a 42-line patch, with comment lines > being half of that, which I'll push once I've looked into the other > concerns which were brought up on this thread. That's good news. > Much of the remaining inefficiancy is how we query for column > information one table at a time (that appears to be around 300ms of the > 900ms or so total time). I'm certainly interested in improving that but > that would require adding more complex data structures to pg_dump than > what we use currently (we'd want to grab all of the columns we care > about in an entire schema and store it locally and then provide a way to > look up that information, etc), so I'm not sure it'd be appropriate to > do now. I'm not sure, either; I'd need to see more to decide. If I were you, I would draft a patch to the point where the community can see the complexity and the performance change. That should be enough to inform the choice among moving forward with the proposed complexity, soliciting other designs, reverting the original changes, or accepting for 9.6 the slowdown as it stands at that time. Other people may have more definite opinions already. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump dump catalog ACLs
Noah, all, * Noah Misch (n...@leadboat.com) wrote: > On Tue, Apr 05, 2016 at 05:50:18PM -0400, Stephen Frost wrote: > > 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) You interpreted the commit message correctly and in a number of cases the correct results are generated, but there's an issue in the WHERE clause being used for some of the object types. The earlier versions of this patch adjusted the WHERE clause by using a sub-select, but later I turned it into a LEFT JOIN and didn't correctly update the WHERE clause to reflect that, so some kinds of ACL changes weren't being picked up. That's relatively straight-forward to fix, but I'm going to put together a series of TAP tests to go with these fixes. While I tested various options and bits and pieces of the code while developing, this really needs a proper test suite that runs through all of these permutations with each change. I'm planning to have that done by the end of the weekend. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] pg_dump dump catalog ACLs
* Noah Misch (n...@leadboat.com) wrote: > On Wed, Apr 20, 2016 at 10:50:21PM -0400, Stephen Frost wrote: > > I'm certainly open to improving these issues now if we agree that they > > should be fixed for 9.6. If we don't want to include such changes in 9.6 > > then I will propose then for post-9.6. > > Folks run clusters with ~1000 databases; we previously accepted at least one > complex performance improvement[1] based on that use case. On the faster of > the two machines I tested, the present thread's commits slowed "pg_dumpall > --schema-only --binary-upgrade" by 1-2s per database. That doubles pg_dump > runtime against the installcheck regression database. A run against a cluster > of one hundred empty databases slowed fifteen-fold, from 8.6s to 131s. > "pg_upgrade -j50" probably will keep things tolerable for the 1000-database > case, but the performance regression remains jarring. I think we should not > release 9.6 with pg_dump performance as it stands today. After looking through the code a bit, I realized that there are a lot of object types which don't have ACLs at all but which exist in pg_catalog and were being analyzed because the bitmask for pg_catalog included ACLs and therefore was non-zero. Clearing that bit for object types which don't have ACLs improved the performance for empty databases quite a bit (from about 3s to a bit under 1s on my laptop). That's a 42-line patch, with comment lines being half of that, which I'll push once I've looked into the other concerns which were brought up on this thread. Much of the remaining inefficiancy is how we query for column information one table at a time (that appears to be around 300ms of the 900ms or so total time). I'm certainly interested in improving that but that would require adding more complex data structures to pg_dump than what we use currently (we'd want to grab all of the columns we care about in an entire schema and store it locally and then provide a way to look up that information, etc), so I'm not sure it'd be appropriate to do now. I'll look into it again once I've addressed the rest of the issues and add the TAP-based tests which I've also been working on to actually get direct testing of pg_dump in the regression suite. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] pg_dump dump catalog ACLs
On Fri, Apr 22, 2016 at 12:25 AM, Noah Misch wrote: > Folks run clusters with ~1000 databases; we previously accepted at least one > complex performance improvement[1] based on that use case. On the faster of > the two machines I tested, the present thread's commits slowed "pg_dumpall > --schema-only --binary-upgrade" by 1-2s per database. That doubles pg_dump > runtime against the installcheck regression database. A run against a cluster > of one hundred empty databases slowed fifteen-fold, from 8.6s to 131s. > "pg_upgrade -j50" probably will keep things tolerable for the 1000-database > case, but the performance regression remains jarring. I think we should not > release 9.6 with pg_dump performance as it stands today. As someone that is responsible for many such clusters, I strongly agree. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump dump catalog ACLs
On Wed, Apr 20, 2016 at 10:50:21PM -0400, Stephen Frost wrote: > On Wednesday, April 20, 2016, Noah Misch wrote: > > On Wed, Apr 20, 2016 at 11:12:44AM -0400, Stephen Frost wrote: > > > > On Sun, Apr 17, 2016 at 11:02:28PM -0400, Noah Misch wrote: > > > > > (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. > > > the additional time for > > > pg_dump is due to the queries looking at the catalog objects and is > > > therefore relatively fixed and is primairly only a large amount of the > > > time when dumping databases which are mostly empty. > > > > Do you think it would be okay to release 9.6 with pg_dump still adding that > > amount of time per database? > > For my 2c, the answer is "yes". I've actually looked at how this could be > improved using a bit of caching in pg_dump for certain things, but I didn't > think those would be appropriate to include in this patch and would be a > general pg_dump performance improvement. > > I'm certainly open to improving these issues now if we agree that they > should be fixed for 9.6. If we don't want to include such changes in 9.6 > then I will propose then for post-9.6. Folks run clusters with ~1000 databases; we previously accepted at least one complex performance improvement[1] based on that use case. On the faster of the two machines I tested, the present thread's commits slowed "pg_dumpall --schema-only --binary-upgrade" by 1-2s per database. That doubles pg_dump runtime against the installcheck regression database. A run against a cluster of one hundred empty databases slowed fifteen-fold, from 8.6s to 131s. "pg_upgrade -j50" probably will keep things tolerable for the 1000-database case, but the performance regression remains jarring. I think we should not release 9.6 with pg_dump performance as it stands today. [1] http://www.postgresql.org/message-id/flat/1718942738eb65c8407fcd864883f...@fuzzy.cz -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump dump catalog ACLs
Noah, On Wednesday, April 20, 2016, Noah Misch wrote: > On Wed, Apr 20, 2016 at 11:12:44AM -0400, Stephen Frost wrote: > > * Noah Misch (n...@leadboat.com ) wrote: > > > On Sun, Apr 17, 2016 at 11:02:28PM -0400, Noah Misch wrote: > > > > (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. > > > > > > [This is a generic notification.] > > > > > > The above-described topic is currently a PostgreSQL 9.6 open item. > Stephen, > > > since you committed the patch believed to have created it, you own > this open > > > item. If that responsibility lies elsewhere, please let us know whose > > > responsibility it is to fix this. Since new open items may be > discovered at > > > any time and I want to plan to have them all fixed well in advance of > the ship > > > date, I will appreciate your efforts toward speedy resolution. Please > > > present, within 72 hours, a plan to fix the defect within seven days > of this > > > message. Thanks. > > > > I'm at PGConf.US but will be reviewing this in detail after. The schema > > qualification will be easily taken care of, the additional time for > > pg_dump is due to the queries looking at the catalog objects and is > > therefore relatively fixed and is primairly only a large amount of the > > time when dumping databases which are mostly empty. > > Do you think it would be okay to release 9.6 with pg_dump still adding that > amount of time per database? > For my 2c, the answer is "yes". I've actually looked at how this could be improved using a bit of caching in pg_dump for certain things, but I didn't think those would be appropriate to include in this patch and would be a general pg_dump performance improvement. I'm certainly open to improving these issues now if we agree that they should be fixed for 9.6. If we don't want to include such changes in 9.6 then I will propose then for post-9.6. Thanks! Stephen
Re: [HACKERS] pg_dump dump catalog ACLs
On Wed, Apr 20, 2016 at 11:12:44AM -0400, Stephen Frost wrote: > * Noah Misch (n...@leadboat.com) wrote: > > On Sun, Apr 17, 2016 at 11:02:28PM -0400, Noah Misch wrote: > > > (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. > > > > [This is a generic notification.] > > > > The above-described topic is currently a PostgreSQL 9.6 open item. Stephen, > > since you committed the patch believed to have created it, you own this open > > item. If that responsibility lies elsewhere, please let us know whose > > responsibility it is to fix this. Since new open items may be discovered at > > any time and I want to plan to have them all fixed well in advance of the > > ship > > date, I will appreciate your efforts toward speedy resolution. Please > > present, within 72 hours, a plan to fix the defect within seven days of this > > message. Thanks. > > I'm at PGConf.US but will be reviewing this in detail after. The schema > qualification will be easily taken care of, the additional time for > pg_dump is due to the queries looking at the catalog objects and is > therefore relatively fixed and is primairly only a large amount of the > time when dumping databases which are mostly empty. Do you think it would be okay to release 9.6 with pg_dump still adding that amount of time per database? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump dump catalog ACLs
Noah, all, * Noah Misch (n...@leadboat.com) wrote: > On Sun, Apr 17, 2016 at 11:02:28PM -0400, Noah Misch wrote: > > 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. > > > (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. > > [This is a generic notification.] > > The above-described topic is currently a PostgreSQL 9.6 open item. Stephen, > since you committed the patch believed to have created it, you own this open > item. If that responsibility lies elsewhere, please let us know whose > responsibility it is to fix this. Since new open items may be discovered at > any time and I want to plan to have them all fixed well in advance of the ship > date, I will appreciate your efforts toward speedy resolution. Please > present, within 72 hours, a plan to fix the defect within seven days of this > message. Thanks. I'm at PGConf.US but will be reviewing this in detail after. The schema qualification will be easily taken care of, the additional time for pg_dump is due to the queries looking at the catalog objects and is therefore relatively fixed and is primairly only a large amount of the time when dumping databases which are mostly empty. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] pg_dump dump catalog ACLs
On Sun, Apr 17, 2016 at 11:02:28PM -0400, Noah Misch wrote: > 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. > (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. [This is a generic notification.] The above-described topic is currently a PostgreSQL 9.6 open item. Stephen, since you committed the patch believed to have created it, you own this open item. If that responsibility lies elsewhere, please let us know whose responsibility it is to fix this. Since new open items may be discovered at any time and I want to plan to have them all fixed well in advance of the ship date, I will appreciate your efforts toward speedy resolution. Please present, within 72 hours, a plan to fix the defect within seven days of this message. Thanks. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump dump catalog ACLs
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
Re: [HACKERS] pg_dump dump catalog ACLs
On Wed, Mar 30, 2016 at 1:35 PM, Jose Luis Tallon wrote: > DESIGN/DOCUMENTATION > * int4 for the "objsubid" field? and int2 would surely suffice, given that we > only allow up to 1600 columns ... if it's a matter of alignment, it would be > interesting to say so somewhere (code comments, maybe?) There is a lot of existing precedent for objsubid being int4, and basically no advantage to making it int2. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 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
Re: [HACKERS] pg_dump dump catalog ACLs
Tom, all, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Joe Conway writes: > > Would it be a terrible idea to add some attribute to ACLs which can be > > used to indicate they should not be dumped (and supporting syntax)? > > Yes, we'd need some way to mark non-null ACLs as being "built-in > defaults". I do not see the need to have SQL syntax supporting that > though. The attached patch does this by adding a 'pg_init_privs' catalog and populating that catalog at the end of initdb, after all of the initial privileges have been set up. > Actually, wouldn't you need to mark individual aclitems as built-in > or not? Consider a situation where we have some function foo() that > by default has EXECUTE permission granted to some built-in "pg_admin" > role. If a given installation then also grants EXECUTE to "joe", > what you really want to have happen is for pg_dump to dump only the > grant to "joe". Mentioning pg_admin's grant would tie the dump to > a particular major PG version's idea of what the built-in roles are, > which is what I'm arguing we need to avoid. What I was thinking about for this was to have pg_dump simply not dump out any GRANTs made to default roles (those starting with 'pg_'), as part of the default roles patch. Another option would be to have pg_dump figure out the delta between what the initial privileges were, as recorded in pg_init_privs, as what the current rights are. I was thinking that the former was simpler, but I don't think it'd be too difficult to do the latter if the consensus is that it's better to do so. > I guess this could also be addressed by having two separate aclitem[] > columns, one that is expected to be frozen after initdb and one for > user-added grants. This is along the lines of what I've done, but I've used a new catalog instead, which is quite small and doesn't complicate or change the regular catalogs. * Joe Conway (m...@joeconway.com) wrote: > On 03/02/2016 12:54 PM, Stephen Frost wrote: > > * Joe Conway (m...@joeconway.com) wrote: > >> On 03/01/2016 08:00 AM, Tom Lane wrote: > >>> Yes, we'd need some way to mark non-null ACLs as being "built-in > >>> defaults". I do not see the need to have SQL syntax supporting that > >>> though. > >> > >> I was thinking the supporting syntax might be used by extensions, for > >> example. > > > > I tend to agree with Tom that we don't really need SQL syntax for this. > > > I don't see any reason it couldn't be used by extensions also, though > > we'd have to do a bit more work on pg_dump to make it actually dump > > out any non-default ACLs for extension-owned objects. > > Without any syntax, what does the extension do, directly insert into > this special catalog table? 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' #5 - Remove 'if (!superuser()) ereport()' calls and REVOKE EXECUTE Thanks! Stephen From 31f4fafeaa06ad7c72ee7c6699253a65ea542d11 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Thu, 10 Mar 2016 20:56:09 -0500 Subject: [PATCH 1/5] Add new catalog called pg_init_privs This new catalog will hold the privileges which the system is initialized with at initdb time. This will allow pg_dump (and any other similar use-cases) to detect when the privileges set on objects in pg_catalog have been changed and handle those changes appropriately. This could be extended to work for extensions also, but that seems like an independent enough change that it should be in a different commit. --- src/backend/catalog/Makefile | 2 +- src/bin/initdb/initdb.c| 121 + src/include/catalog/indexing.h | 3 + src/include/catalog/pg_init_privs.h| 83 src/test/regress/expected/sanity_check.out | 1 + 5 files changed, 209 insertions(+), 1 deletion(-) create mode 100644 src/include/catalog/pg_init_privs.h diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile index 25130ec..1ce7610 100644 --- a/src/backend/catalog/Makefile +++ b/src/backend/catalog/Makefile @@ -40,7 +40,7 @@ POSTGRES_BKI_SRCS = $(addprefix $
Re: [HACKERS] pg_dump dump catalog ACLs
* Joe Conway (m...@joeconway.com) wrote: > On 03/02/2016 12:54 PM, Stephen Frost wrote: > > * Joe Conway (m...@joeconway.com) wrote: > >> On 03/01/2016 08:00 AM, Tom Lane wrote: > >>> Yes, we'd need some way to mark non-null ACLs as being "built-in > >>> defaults". I do not see the need to have SQL syntax supporting that > >>> though. > >> > >> I was thinking the supporting syntax might be used by extensions, for > >> example. > > > > I tend to agree with Tom that we don't really need SQL syntax for this. > > > I don't see any reason it couldn't be used by extensions also, though > > we'd have to do a bit more work on pg_dump to make it actually dump > > out any non-default ACLs for extension-owned objects. > > Without any syntax, what does the extension do, directly insert into > this special catalog table? Perhaps nothing- we already are tracking everything they've created, at the end of the extension's script, we could simply go over all of the objects which are part of the extension and save off the non-NULL ACLs which exist. * David G. Johnston (david.g.johns...@gmail.com) wrote: > On Wed, Mar 2, 2016 at 2:44 PM, Joe Conway wrote: > > > On 03/02/2016 12:54 PM, Stephen Frost wrote: > > > * Joe Conway (m...@joeconway.com) wrote: > > >> On 03/01/2016 08:00 AM, Tom Lane wrote: > > >>> Yes, we'd need some way to mark non-null ACLs as being "built-in > > >>> defaults". I do not see the need to have SQL syntax supporting that > > >>> though. > > >> > > >> I was thinking the supporting syntax might be used by extensions, for > > >> example. > > > > > > I tend to agree with Tom that we don't really need SQL syntax for this. > > > > > I don't see any reason it couldn't be used by extensions also, though > > > we'd have to do a bit more work on pg_dump to make it actually dump > > > out any non-default ACLs for extension-owned objects. > > > > Without any syntax, what does the extension do, directly insert into > > this special catalog table? > > > > > The desire in the thread I linked was for the user, not the extension, to > alter the permissions. In that context (and possibly here as well) > PostgreSQL would (somehow?) recognize the target as being special and thus > requiring adding or updating an entry into the supplemental catalog table > when the usual GRANT/REVOKE SQL command is issued. Not quite. The idea here is for us to track in the catalog what the ACLs were at the "start" (being "initdb" time for the database as a whole, and "CREATE EXTENSION" time for the extension) and then dump out any ACLs which have been changed since then. That strikes me as much simpler than having to track every GRANT/REVOKE done against some special set of objects.. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] pg_dump dump catalog ACLs
On Wed, Mar 2, 2016 at 2:44 PM, Joe Conway wrote: > On 03/02/2016 12:54 PM, Stephen Frost wrote: > > * Joe Conway (m...@joeconway.com) wrote: > >> On 03/01/2016 08:00 AM, Tom Lane wrote: > >>> Yes, we'd need some way to mark non-null ACLs as being "built-in > >>> defaults". I do not see the need to have SQL syntax supporting that > >>> though. > >> > >> I was thinking the supporting syntax might be used by extensions, for > >> example. > > > > I tend to agree with Tom that we don't really need SQL syntax for this. > > > I don't see any reason it couldn't be used by extensions also, though > > we'd have to do a bit more work on pg_dump to make it actually dump > > out any non-default ACLs for extension-owned objects. > > Without any syntax, what does the extension do, directly insert into > this special catalog table? > > The desire in the thread I linked was for the user, not the extension, to alter the permissions. In that context (and possibly here as well) PostgreSQL would (somehow?) recognize the target as being special and thus requiring adding or updating an entry into the supplemental catalog table when the usual GRANT/REVOKE SQL command is issued. In effect any object dependent upon an EXTENSION or that already exists in this special catalog table would need to have the supplemental procedure executed. David J.
Re: [HACKERS] pg_dump dump catalog ACLs
On 03/02/2016 12:54 PM, Stephen Frost wrote: > * Joe Conway (m...@joeconway.com) wrote: >> On 03/01/2016 08:00 AM, Tom Lane wrote: >>> Yes, we'd need some way to mark non-null ACLs as being "built-in >>> defaults". I do not see the need to have SQL syntax supporting that >>> though. >> >> I was thinking the supporting syntax might be used by extensions, for >> example. > > I tend to agree with Tom that we don't really need SQL syntax for this. > I don't see any reason it couldn't be used by extensions also, though > we'd have to do a bit more work on pg_dump to make it actually dump > out any non-default ACLs for extension-owned objects. Without any syntax, what does the extension do, directly insert into this special catalog table? Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
Re: [HACKERS] pg_dump dump catalog ACLs
* David G. Johnston (david.g.johns...@gmail.com) wrote: > On Wed, Mar 2, 2016 at 1:54 PM, Stephen Frost wrote: > > Rather than have two aclitem[] columns in every catalog, since this > > information is only used by pg_dump and not during normal operation, we > > could use the approach that pg_description took and have an independent > > catalog table which just contains all non-NULL "system" ACLs. We could > > populate it at the bottom of system_views.sql, so that we don't have to > > explicitly think about updating that table whenever there's a change to > > what the default ACLs are. > > > > I don't see any reason it couldn't be used by extensions also, though > > we'd have to do a bit more work on pg_dump to make it actually dump > > out any non-default ACLs for extension-owned objects. > > > > It sounds like this train of thought would resolve this complaint? > > > http://www.postgresql.org/message-id/CADmxfmmz-ATwptaidTSAF0XE=cpeikmyc00sj6t9xf6kcv5...@mail.gmail.com > > Namely allowing users to edit permissions on extension objects and have > those changes dumped and then restored after the dependent CREATE EXTENSION > command is executed during pg_restore. > > Did I interpret that right? Yes, I was following that thread also (as was Joe, I imagine) and that's the idea. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] pg_dump dump catalog ACLs
On Wed, Mar 2, 2016 at 1:54 PM, Stephen Frost wrote: > * Joe Conway (m...@joeconway.com) wrote: > > On 03/01/2016 08:00 AM, Tom Lane wrote: > > > Joe Conway writes: > > >> Would it be a terrible idea to add some attribute to ACLs which can be > > >> used to indicate they should not be dumped (and supporting syntax)? > > > > > > Yes, we'd need some way to mark non-null ACLs as being "built-in > > > defaults". I do not see the need to have SQL syntax supporting that > > > though. > > > > I was thinking the supporting syntax might be used by extensions, for > > example. > > I tend to agree with Tom that we don't really need SQL syntax for this. > > > > Actually, wouldn't you need to mark individual aclitems as built-in > > > or not? Consider a situation where we have some function foo() that > > > by default has EXECUTE permission granted to some built-in "pg_admin" > > > role. If a given installation then also grants EXECUTE to "joe", > > > what you really want to have happen is for pg_dump to dump only the > > > grant to "joe". Mentioning pg_admin's grant would tie the dump to > > > a particular major PG version's idea of what the built-in roles are, > > > which is what I'm arguing we need to avoid. > > > > Yes, I guess it would need to be a per aclitem attribute. > > Agreed. > > > > I guess this could also be addressed by having two separate aclitem[] > > > columns, one that is expected to be frozen after initdb and one for > > > user-added grants. > > > > Yeah, that would work, but seems kind of ugly. > > Rather than have two aclitem[] columns in every catalog, since this > information is only used by pg_dump and not during normal operation, we > could use the approach that pg_description took and have an independent > catalog table which just contains all non-NULL "system" ACLs. We could > populate it at the bottom of system_views.sql, so that we don't have to > explicitly think about updating that table whenever there's a change to > what the default ACLs are. > > I don't see any reason it couldn't be used by extensions also, though > we'd have to do a bit more work on pg_dump to make it actually dump > out any non-default ACLs for extension-owned objects. > It sounds like this train of thought would resolve this complaint? http://www.postgresql.org/message-id/CADmxfmmz-ATwptaidTSAF0XE=cpeikmyc00sj6t9xf6kcv5...@mail.gmail.com Namely allowing users to edit permissions on extension objects and have those changes dumped and then restored after the dependent CREATE EXTENSION command is executed during pg_restore. Did I interpret that right? David J.
Re: [HACKERS] pg_dump dump catalog ACLs
* Joe Conway (m...@joeconway.com) wrote: > On 03/01/2016 08:00 AM, Tom Lane wrote: > > Joe Conway writes: > >> Would it be a terrible idea to add some attribute to ACLs which can be > >> used to indicate they should not be dumped (and supporting syntax)? > > > > Yes, we'd need some way to mark non-null ACLs as being "built-in > > defaults". I do not see the need to have SQL syntax supporting that > > though. > > I was thinking the supporting syntax might be used by extensions, for > example. I tend to agree with Tom that we don't really need SQL syntax for this. > > Actually, wouldn't you need to mark individual aclitems as built-in > > or not? Consider a situation where we have some function foo() that > > by default has EXECUTE permission granted to some built-in "pg_admin" > > role. If a given installation then also grants EXECUTE to "joe", > > what you really want to have happen is for pg_dump to dump only the > > grant to "joe". Mentioning pg_admin's grant would tie the dump to > > a particular major PG version's idea of what the built-in roles are, > > which is what I'm arguing we need to avoid. > > Yes, I guess it would need to be a per aclitem attribute. Agreed. > > I guess this could also be addressed by having two separate aclitem[] > > columns, one that is expected to be frozen after initdb and one for > > user-added grants. > > Yeah, that would work, but seems kind of ugly. Rather than have two aclitem[] columns in every catalog, since this information is only used by pg_dump and not during normal operation, we could use the approach that pg_description took and have an independent catalog table which just contains all non-NULL "system" ACLs. We could populate it at the bottom of system_views.sql, so that we don't have to explicitly think about updating that table whenever there's a change to what the default ACLs are. I don't see any reason it couldn't be used by extensions also, though we'd have to do a bit more work on pg_dump to make it actually dump out any non-default ACLs for extension-owned objects. Thoughts? Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] pg_dump dump catalog ACLs
On 03/01/2016 08:00 AM, Tom Lane wrote: > Joe Conway writes: >> Would it be a terrible idea to add some attribute to ACLs which can be >> used to indicate they should not be dumped (and supporting syntax)? > > Yes, we'd need some way to mark non-null ACLs as being "built-in > defaults". I do not see the need to have SQL syntax supporting that > though. I was thinking the supporting syntax might be used by extensions, for example. > Actually, wouldn't you need to mark individual aclitems as built-in > or not? Consider a situation where we have some function foo() that > by default has EXECUTE permission granted to some built-in "pg_admin" > role. If a given installation then also grants EXECUTE to "joe", > what you really want to have happen is for pg_dump to dump only the > grant to "joe". Mentioning pg_admin's grant would tie the dump to > a particular major PG version's idea of what the built-in roles are, > which is what I'm arguing we need to avoid. Yes, I guess it would need to be a per aclitem attribute. > I guess this could also be addressed by having two separate aclitem[] > columns, one that is expected to be frozen after initdb and one for > user-added grants. Yeah, that would work, but seems kind of ugly. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
Re: [HACKERS] pg_dump dump catalog ACLs
Joe Conway writes: > Would it be a terrible idea to add some attribute to ACLs which can be > used to indicate they should not be dumped (and supporting syntax)? Yes, we'd need some way to mark non-null ACLs as being "built-in defaults". I do not see the need to have SQL syntax supporting that though. Actually, wouldn't you need to mark individual aclitems as built-in or not? Consider a situation where we have some function foo() that by default has EXECUTE permission granted to some built-in "pg_admin" role. If a given installation then also grants EXECUTE to "joe", what you really want to have happen is for pg_dump to dump only the grant to "joe". Mentioning pg_admin's grant would tie the dump to a particular major PG version's idea of what the built-in roles are, which is what I'm arguing we need to avoid. I guess this could also be addressed by having two separate aclitem[] columns, one that is expected to be frozen after initdb and one for user-added grants. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump dump catalog ACLs
On 02/29/2016 08:52 PM, Tom Lane wrote: > Stephen Frost writes: >> As it turns out, there isn't such an issue as the default for functions >> is to allow PUBLIC to EXECUTE and therefore we don't dump out ACLs for >> most functions. The follow-on change to this patch is to modify those >> functions to *not* have the default/NULL ACL (and also drop the explicit >> if (!superuser()) ereport() checks in those functions), which will work >> just fine and won't be overwritten during pg_upgrade because those >> functions currently just have the default ACL, which we don't dump out. > > Yes, so it would probably manage to not fail during 9.6 -> 9.7 migration. > But you *won't ever again* get to change the default ACLs on those > functions. That does not seem like a great bet from here. Would it be a terrible idea to add some attribute to ACLs which can be used to indicate they should not be dumped (and supporting syntax)? -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
Re: [HACKERS] pg_dump dump catalog ACLs
Stephen Frost writes: > * Tom Lane (t...@sss.pgh.pa.us) wrote: >> No, the point of it would be to not have pg_dump scripts overriding >> installed-by-default ACLs. A newer PG version might have different >> ideas about what those should be. I don't think this is exactly an >> academic concern, either: wouldn't a likely outcome of your default-roles >> work be that some built-in functions have different initial ACLs than >> they do today? Good luck with that, if pg_upgrade overwrites those >> ACLs with the previous-version values. > As it turns out, there isn't such an issue as the default for functions > is to allow PUBLIC to EXECUTE and therefore we don't dump out ACLs for > most functions. The follow-on change to this patch is to modify those > functions to *not* have the default/NULL ACL (and also drop the explicit > if (!superuser()) ereport() checks in those functions), which will work > just fine and won't be overwritten during pg_upgrade because those > functions currently just have the default ACL, which we don't dump out. Yes, so it would probably manage to not fail during 9.6 -> 9.7 migration. But you *won't ever again* get to change the default ACLs on those functions. That does not seem like a great bet from here. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump dump catalog ACLs
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > * Tom Lane (t...@sss.pgh.pa.us) wrote: > >> To make this work, you'd need a way to distinguish privileges installed > >> by initdb from those changed later. > > > To replicate whatever the current ACL is, we don't actually need to > > make such a differentiation. I'm not against doing so, but the only > > point of it would be to eliminate a few extra lines being dumped out > > which re-run those commands that initdb runs on restore. > > No, the point of it would be to not have pg_dump scripts overriding > installed-by-default ACLs. A newer PG version might have different > ideas about what those should be. I don't think this is exactly an > academic concern, either: wouldn't a likely outcome of your default-roles > work be that some built-in functions have different initial ACLs than > they do today? Good luck with that, if pg_upgrade overwrites those > ACLs with the previous-version values. As it turns out, there isn't such an issue as the default for functions is to allow PUBLIC to EXECUTE and therefore we don't dump out ACLs for most functions. The follow-on change to this patch is to modify those functions to *not* have the default/NULL ACL (and also drop the explicit if (!superuser()) ereport() checks in those functions), which will work just fine and won't be overwritten during pg_upgrade because those functions currently just have the default ACL, which we don't dump out. Of course, it's a different story if the user changes the ACL on objects in pg_catalog and then we change what we think the default ACL should be, but in such a case, I'm guessing we should probably go with what the user explicitly asked for anyway and if there's a serious enough change in the permissions of the function then perhaps we should have a different function instead of re-defining the existing one. We do have some fun issues with pg_upgrade by going with this approach of having pg_dump dump out ACLs- what happens when there's a function or column which goes away? If there's a non-NULL ACL on them, the restore will just outright fail, if we don't do something more. I'm not a huge fan of coding into pg_dump the knowledge of every object which exists for every version of PG we support for pg_dump though. Regarding the default roles patch, now that it's down to only one default role, based on the assumption that this approach with pg_dump will solve all the other concerns, there isn't really much overlap between the default roles and the function ACLs. Those functions which will be able to work with just ACLs won't have a default role and the functions which we need a default role for will have the default NULL ACL. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] pg_dump dump catalog ACLs
Stephen Frost writes: > * Tom Lane (t...@sss.pgh.pa.us) wrote: >> To make this work, you'd need a way to distinguish privileges installed >> by initdb from those changed later. > To replicate whatever the current ACL is, we don't actually need to > make such a differentiation. I'm not against doing so, but the only > point of it would be to eliminate a few extra lines being dumped out > which re-run those commands that initdb runs on restore. No, the point of it would be to not have pg_dump scripts overriding installed-by-default ACLs. A newer PG version might have different ideas about what those should be. I don't think this is exactly an academic concern, either: wouldn't a likely outcome of your default-roles work be that some built-in functions have different initial ACLs than they do today? Good luck with that, if pg_upgrade overwrites those ACLs with the previous-version values. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump dump catalog ACLs
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > Per discussion about the best approach to reduce the amount of > > superuser-only capabilities, this patch modifies pg_dump to dump out > > all ACLs which exist on objects in the pg_catalog schema. > > Um ... surely there are some of those that are installed by default? There are a few, but not terribly many currently. > To make this work, you'd need a way to distinguish privileges installed > by initdb from those changed later. To replicate whatever the current ACL is, we don't actually need to make such a differentiation. I'm not against doing so, but the only point of it would be to eliminate a few extra lines being dumped out which re-run those commands that initdb runs on restore. The downside of doing so would be having to keep track of the exact ACLs set for every object in pg_catalog which has a non-NULL ACL at initdb time for every version of PG that the latest version of pg_dump supports, and making sure that any changes to those get updated in pg_dump in addition to the relevant system_views.sql change. That's possible, but I wasn't sure it was worth it. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] pg_dump dump catalog ACLs
Stephen Frost writes: > Per discussion about the best approach to reduce the amount of > superuser-only capabilities, this patch modifies pg_dump to dump out > all ACLs which exist on objects in the pg_catalog schema. Um ... surely there are some of those that are installed by default? To make this work, you'd need a way to distinguish privileges installed by initdb from those changed later. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_dump dump catalog ACLs
All, Per discussion about the best approach to reduce the amount of superuser-only capabilities, this patch modifies pg_dump to dump out all ACLs which exist on objects in the pg_catalog schema. With this change, follow-on trivial patches will remove explicit superuser() checks from functions and replace them with 'REVOKE EXECUTE FROM public' commands, allowing users to then control what users are allowed to execute those functions. Started as a new thread to hopefully gain more interest. Will be registered in the March commitfest shortly. Thanks! Stephen From b2b01b498f3d9fede2e876785effd48f00feee34 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Mon, 29 Feb 2016 21:11:46 -0500 Subject: [PATCH] Make pg_dump dump ACLs for pg_catalog objects Historically, we've avoided dumping anything about the objects in the pg_catalog schema. Unfortunately, this has meant that any changes or even initial ACLs set for objects in pg_catalog are not preserved. Instead, dump out the ACLs which are set on objects in pg_catalog in the same way we dump ACLs for user objects. This is implemented by adding the notion of a 'dump component' (such as an ACL, or comments, or policies) which can be requested to be dumped out rather than everything. --- src/bin/pg_dump/common.c |2 +- src/bin/pg_dump/pg_dump.c | 1576 - src/bin/pg_dump/pg_dump.h | 14 +- 3 files changed, 865 insertions(+), 727 deletions(-) diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c index f798b15..507b0bf 100644 --- a/src/bin/pg_dump/common.c +++ b/src/bin/pg_dump/common.c @@ -437,7 +437,7 @@ AssignDumpId(DumpableObject *dobj) dobj->dumpId = ++lastDumpId; dobj->name = NULL; /* must be set later */ dobj->namespace = NULL; /* may be set later */ - dobj->dump = true; /* default assumption */ + dobj->dump = DUMP_COMPONENT_ALL; /* default assumption */ dobj->ext_member = false; /* default assumption */ dobj->dependencies = NULL; dobj->nDeps = 0; diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 64c2673..416e6a7 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -1284,7 +1284,7 @@ checkExtensionMembership(DumpableObject *dobj, DumpOptions *dopt) * extension contents with something different. */ if (!dopt->binary_upgrade) - dobj->dump = false; + dobj->dump = DUMP_COMPONENT_NONE; else dobj->dump = ext->dobj.dump; @@ -1306,16 +1306,20 @@ selectDumpableNamespace(NamespaceInfo *nsinfo, DumpOptions *dopt) * namespaces. If specific namespaces are being dumped, dump just those * namespaces. Otherwise, dump all non-system namespaces. */ + if (table_include_oids.head != NULL) - nsinfo->dobj.dump = false; + nsinfo->dobj.dump = DUMP_COMPONENT_NONE; else if (schema_include_oids.head != NULL) nsinfo->dobj.dump = simple_oid_list_member(&schema_include_oids, - nsinfo->dobj.catId.oid); + nsinfo->dobj.catId.oid) ? + DUMP_COMPONENT_ALL : DUMP_COMPONENT_NONE; + else if (strncmp(nsinfo->dobj.name, "pg_catalog", 10) == 0) + nsinfo->dobj.dump = DUMP_COMPONENT_ACL; else if (strncmp(nsinfo->dobj.name, "pg_", 3) == 0 || strcmp(nsinfo->dobj.name, "information_schema") == 0) - nsinfo->dobj.dump = false; + nsinfo->dobj.dump = DUMP_COMPONENT_NONE; else - nsinfo->dobj.dump = true; + nsinfo->dobj.dump = DUMP_COMPONENT_ALL; /* * In any case, a namespace can be excluded by an exclusion switch @@ -1323,7 +1327,7 @@ selectDumpableNamespace(NamespaceInfo *nsinfo, DumpOptions *dopt) if (nsinfo->dobj.dump && simple_oid_list_member(&schema_exclude_oids, nsinfo->dobj.catId.oid)) - nsinfo->dobj.dump = false; + nsinfo->dobj.dump = DUMP_COMPONENT_NONE; } /* @@ -1342,7 +1346,8 @@ selectDumpableTable(TableInfo *tbinfo, DumpOptions *dopt) */ if (table_include_oids.head != NULL) tbinfo->dobj.dump = simple_oid_list_member(&table_include_oids, - tbinfo->dobj.catId.oid); + tbinfo->dobj.catId.oid) ? +DUMP_COMPONENT_ALL : DUMP_COMPONENT_NONE; else tbinfo->dobj.dump = tbinfo->dobj.namespace->dobj.dump; @@ -1352,7 +1357,7 @@ selectDumpableTable(TableInfo *tbinfo, DumpOptions *dopt) if (tbinfo->dobj.dump && simple_oid_list_member(&table_exclude_oids, tbinfo->dobj.catId.oid)) - tbinfo->dobj.dump = false; + tbinfo->dobj.dump = DUMP_COMPONENT_NONE; } /* @@ -1381,7 +1386,7 @@ selectDumpableType(TypeInfo *tyinfo, DumpOptions *dopt) if (tytable != NULL) tyinfo->dobj.dump = tytable->dobj.dump; else - tyinfo->dobj.dump = false; + tyinfo->dobj.dump = DUMP_COMPONENT_NONE; return; } @@ -1401,11 +1406,7 @@ selectDumpableType(TypeInfo *tyinfo, DumpOptions *dopt) if (checkExtensionMembership(&tyinfo->dobj, dopt)) return; /* extension membership overrides all else */ - /* dump only types in dumpable namespaces */ - if (!tyinfo->dobj.namespace->dobj.dump) - tyinfo->dobj.dump = false;