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

Stephen Frost <sfrost@snowman.net> 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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> 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

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@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

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@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

On 02/29/2016 08:52 PM, Tom Lane wrote:
> Stephen Frost <sfrost@snowman.net> 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

Joe Conway <mail@joeconway.com> 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

On 03/01/2016 08:00 AM, Tom Lane wrote:
> Joe Conway <mail@joeconway.com> 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

* Joe Conway (mail@joeconway.com) wrote:
> On 03/01/2016 08:00 AM, Tom Lane wrote:
> > Joe Conway <mail@joeconway.com> 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

* David G. Johnston (david.g.johnston@gmail.com) wrote:
> On Wed, Mar 2, 2016 at 1:54 PM, Stephen Frost <sfrost@snowman.net> 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=cPeikMyc00sj6t9xF6KCV5jCQ@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

On 03/02/2016 12:54 PM, Stephen Frost wrote:
> * Joe Conway (mail@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

* Joe Conway (mail@joeconway.com) wrote:
> On 03/02/2016 12:54 PM, Stephen Frost wrote:
> > * Joe Conway (mail@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.johnston@gmail.com) wrote:
> On Wed, Mar 2, 2016 at 2:44 PM, Joe Conway <mail@joeconway.com> wrote:
>
> > On 03/02/2016 12:54 PM, Stephen Frost wrote:
> > > * Joe Conway (mail@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

Tom, all,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Joe Conway <mail@joeconway.com> 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 (mail@joeconway.com) wrote:
> On 03/02/2016 12:54 PM, Stephen Frost wrote:
> > * Joe Conway (mail@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

Alexander,

* Alexander Korotkov (a.korotkov@postgrespro.ru) wrote:
> 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.

Fantastic!  Many thanks!

> On Mon, Mar 14, 2016 at 7:00 PM, Stephen Frost <sfrost@snowman.net> wrote:
>
> > I've not included it in this patch, but my thinking here would be to add
> > a check to the GRANT functions which checks 'if (creating_extension)'
> > and records/updates the entries in pg_init_privs for those objects.
> > This is similar to how we handle dependencies for extensions currently.
> > That way, extensions don't have to do anything and if the user changes
> > the permissions on an extension's object, the permissions for that
> > object would then be dumped out.
> >
> > This still requires more testing, documentation, and I'll see about
> > making it work completely for extensions also, but wanted to provide an
> > update and solicit for review/comments.
> >
> > The patches have been broken up in what I hope is a logical way for
> > those who wish to review/comment:
> >
> > #1 - Add the pg_init_privs catalog
> > #2 - Change pg_dump to use a bitmap instead of boolean for 'dump'
> > #3 - Split 'dump' into 'dump' and 'dump_contains'
> > #4 - Make pg_dump include changed ACLs in dump of 'pg_catalog'
> >
>
> +  "CASE WHEN pip.initprivs IS NOT DISTINCT FROM n.nspacl "
> +  "THEN false ELSE true END as dumpacl "
>
> Why don't just write " pip.initprivs IS DISTINCT FROM n.nspacl AS dumpacl"?
> I think there are few places whene CASE could be eliminated.

Unfortunately, the reality is quite the opposite, as I expect you'll see
from the latest version of this patch.  Apologies for not posting a
newer patch sooner, but I've continued to work on it since my last post.

In particular, I've now added support for handling initial privileges of
extensions and dealt with both the addition of privileges but also the
revocation of the inital ones the object had.  I also had to make
adjustments for dealing with binary-upgrade mode with extensions.

The attached also properly splits out the ACL arrays into individual
items for consideration, so that we don't end up in odd cases where
there appears to be a difference due to a change in the ordering
(perhaps someone revoked a privilege which existed initially, only to
add it back identically later, but which would put it at the end of the
array).

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

That wasn't intentional and was caught a couple days ago while I was
reviewing the patch and working through adding extension support, many
thanks for taking a look through the patch and pointing it out though,
definitely a good catch.

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

We're in pg_dump here, which could be working against different major
versions of PG, so I'd rather avoid doing that.  Also, I've added a ton
of comments across the patch which hopefully help explain what's going
on, along with some of the documentation which will be necessary (though
there's more to do on that front, in particular when it comes to
pg_dump).

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

I'd argue that the biggest piece that needs better testing is pg_dump,
but we don't have a very good framework for testing it currently during
the regression tests.  We do run pg_upgrade and that includes dumping
out the regression database, which is good, and that all passes
currently, and I've tested that doing a pg_dump against all versions
of PG which I could compile easily on my Ubuntu 15.10 laptop (7.4 and
above) of their regression databases at least doesn't fail and a bit of
checking by hand leads me to believe that they look reasonable.

I've also made changes to the initdb'd ACLs (granting and revoking them)
and gotten the expected results from pg_dump, and played around a bit
with doing the same for an extension (pg_buffercache, if you're
curious) and that appeared to all work.

Attached is a new version to look at and play with.  I'm not entirely
thrilled with some of the hacking that was necessary to make
buildACLCommands not throw in extra (and now unnecessary) commands and
that deserves more comments (and I'm a bit suspicious that there's
actually an unintentional bug which actually ends up doing the right
thing, but not for the right reasons, when it comes to how the 'owner'
case is handled...).  There's also more to be said when it comes to the
use of acldefault(), I expect, though I was having some serious
difficulty coming up with a better answer (suggestions welcome...).

In any case, it's awful late, but wanted to show where I'm currently at
with all of this, so my apologies in advance for any grossly incorrect
statements, comments, or code.  I look forward to comments and will
attempt to address them tomorrow night, otherwise I'll probably be back
looking this over again myself.

The complications of this approach are, impressively, worse than I had
expected them to be, and I thought they'd be pretty bad.

Thanks!

Stephen

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

Patch does not apply cleanly (via "git am") as of today's master; but does apply with "patch"; minor fuzzes apart, so
testedwith that.
 
The functionality contained in this patch was badly needed: IMHO, it is a BUG that we do not *perfectly* restore (even
whenit's only default and catalog/extensions)
 

DESIGN/DOCUMENTATION
* int4 for the "objsubid" field? and int2 would surely suffice, given that we only allow up to 1600 columns ... if it's
amatter of alignment, it would be interesting to say so somewhere (code comments, maybe?)
 
* Initial ACL preservation:
"       * Note that any initial ACLs (see pg_init_privs) will be removed when we       * extract the information about
theobject.  We don't provide support for       * initial policies and security labels and it seems unlikely for those
   * to ever exist, but we may have to revisit this later.
 
" ... fair enough, but it ought to be documented in the manual, too. IMO.


DOCUMENTATION
- Under "privtype" it says:  "A code defining the type of initial privilege of this object; see text" ... but the patch
doesn'tseem to contain the referenced text¿?A minor omission, definitively --- it is indeed easily derived from enum
InitPrivsType'sdefinition at include/catalog/pg_init_privs.h
 

CODE
"#if ACLDEBUG" code blocks are still present; should be removed before commiting  (I quite like the DumpComponents
typedefvs bool [previous], BTW; Clean, readable and effective)
 

Code comments are descriptive enough and generally properly spellt. Exception:  src/bin/pg_dump/dumputils.c:
s/privilegs/privileges/ (line 132)
 

Minor issue: diff size could have been really reduced by omitting the extra indent after the newly-included checks for
"dumpingrequired"   (i.e. not indent the dumpXXX after the "if ( yyy->dobj.dump & ... )"   After all, pgindent will
takecare of tabs before release :)
 

Within buildACLCommands(), multiple ocurrences of strncmp( ... ,"group ", strlen("group ") could have been avoided, but
codesimplicity/readability could be affected... whereas in other places one reads "if( strncmp(nsinfo->dobj.name,
"pg_catalog",10) == 0) "
 

I think it is debatable whether DumpOptions belong inside Archive or not (I would have kept it separate, FWIW), but
thatis unrelated to the correctness of this patch.
 

Code builds, installs and "make check"s ok.

TESTING:- pg_dumpall properly dumps the REVOKE statements to store the ACL changes to catalog objects- pg_init_privs
recordsall initial privileges on objects (from initdb time)- "psql -f" re-creates all privileges 

The new status of this patch is: Ready for Committer

On Wed, Mar 30, 2016 at 1:35 PM, Jose Luis Tallon
<jltallon@adv-solutions.net> wrote:
> DESIGN/DOCUMENTATION
> * int4 for the "objsubid" field? and int2 would surely suffice, given that we only allow up to 1600 columns ... if
it'sa 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

Jose,

* Jose Luis Tallon (jltallon@adv-solutions.net) wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            tested, passed

Thanks for the review!

> Patch does not apply cleanly (via "git am") as of today's master; but does apply with "patch"; minor fuzzes apart, so
testedwith that. 

Just needed to be rebased and then updated to reflect the additions of
dumping the access methods to pg_dump.  Updated patch series attached.

> The functionality contained in this patch was badly needed: IMHO, it is a BUG that we do not *perfectly* restore
(evenwhen it's only default and catalog/extensions) 

Guess I have to agree with that. :)

> DESIGN/DOCUMENTATION
> * int4 for the "objsubid" field? and int2 would surely suffice, given that we only allow up to 1600 columns ... if
it'sa matter of alignment, it would be interesting to say so somewhere (code comments, maybe?) 

As Robert noted, this is a long-standing convention and is used
throughout our existing code-base.  It's certainly not the purview of
this particular patch to change that.

> * Initial ACL preservation:
> "       * Note that any initial ACLs (see pg_init_privs) will be removed when we
>         * extract the information about the object.  We don't provide support for
>         * initial policies and security labels and it seems unlikely for those
>         * to ever exist, but we may have to revisit this later.
> " ... fair enough, but it ought to be documented in the manual, too. IMO.

Fixed.

> DOCUMENTATION
> - Under "privtype" it says:  "A code defining the type of initial privilege of this object; see text" ... but the
patchdoesn't seem to contain the referenced text¿? 
>  A minor omission, definitively --- it is indeed easily derived from enum InitPrivsType's definition at
include/catalog/pg_init_privs.h

Fixed.

> CODE
> "#if ACLDEBUG" code blocks are still present; should be removed before commiting
>    (I quite like the DumpComponents typedef vs bool [previous], BTW; Clean, readable and effective)

The '#if ACLDEBUG's are not from this patch-set and I don't think it's
this patch's job to remove them ...

> Code comments are descriptive enough and generally properly spellt.
>   Exception:  src/bin/pg_dump/dumputils.c:   s/privilegs/privileges/  (line 132)

Fixed.

> Minor issue: diff size could have been really reduced by omitting the extra indent after the newly-included checks
for"dumping required" 
>     (i.e. not indent the dumpXXX after the "if ( yyy->dobj.dump & ... )"
>     After all, pgindent will take care of tabs before release :)

Fixed as best as I could, pgindent is pretty clean for the changes which
this patch set introduces (note that other patches have been committed
which are not pgindent clean and therefore certain files will still be
changed on the next pgindent run, such as pg_dump.c).

> Within buildACLCommands(), multiple ocurrences of strncmp( ... ,"group ", strlen("group ") could have been avoided,
butcode simplicity/readability could be affected 
>  ... whereas in other places one reads "if( strncmp(nsinfo->dobj.name, "pg_catalog", 10) == 0) "

Fixed by doing 'strlen("pg_catalog")' in the one spot which had been
using a hard-coded value of '10'.

> I think it is debatable whether DumpOptions belong inside Archive or not (I would have kept it separate, FWIW), but
thatis unrelated to the correctness of this patch. 

I didn't see the sense of changing that as part of this patch.

> Code builds, installs and "make check"s ok.
>
> TESTING:
>  - pg_dumpall properly dumps the REVOKE statements to store the ACL changes to catalog objects
>  - pg_init_privs records all initial privileges on objects (from initdb time)
>  - "psql -f" re-creates all privileges
>
> The new status of this patch is: Ready for Committer

Very cool.

Thanks again for the review!

I invite others to take a look at the new catalog ('pg_init_privs') and
the changes made to pg_dump and voice any concerns or opinions regarding
those or anything else in this patch set as I've now been through it
again, cleaned up and added more comments, cleaned up and added more to
the documentation, even found a couple bugs which I fixed (particularly
around populating pg_init_privs for columns, and in buildACLCommands()
in pg_dump which is a bit tricky, but now has a lot more comments) and
generally am feeling pretty happy about this patch set.

Barring objections, I'm planning to push this in the next few days.

Thanks!

Stephen

On 04/02/2016 03:23 AM, Stephen Frost wrote: [snip]
>> Patch does not apply cleanly (via "git am") as of today's master; but does apply with "patch"; minor fuzzes apart,
sotested with that.
 
> Just needed to be rebased and then updated to reflect the additions of
> dumping the access methods to pg_dump.  Updated patch series attached.

Confirmed clean, thanks! :)


> * Initial ACL preservation:
> "       * Note that any initial ACLs (see pg_init_privs) will be removed when we
>          * extract the information about the object.  We don't provide support for
>          * initial policies and security labels and it seems unlikely for those
>          * to ever exist, but we may have to revisit this later.
> " ... fair enough, but it ought to be documented in the manual, too. IMO.
> Fixed.

Check.

> DOCUMENTATION
> - Under "privtype" it says:  "A code defining the type of initial privilege of this object; see text" ... but the
patchdoesn't seem to contain the referenced text¿?
 
>   A minor omission, definitively --- it is indeed easily derived from enum InitPrivsType's definition at
include/catalog/pg_init_privs.h
> Fixed.

Check

[snip]
>> Within buildACLCommands(), multiple ocurrences of strncmp( ... ,"group ", strlen("group ") could have been avoided,
butcode simplicity/readability could be affected
 
>>   ... whereas in other places one reads "if( strncmp(nsinfo->dobj.name, "pg_catalog", 10) == 0) "
> Fixed by doing 'strlen("pg_catalog")' in the one spot which had been
> using a hard-coded value of '10'.

Ok. I tend to do  "....  ,10 /*strlen("pg_catalog")*/ , ... " instead, 
but this is definitively good.

> [snip]
> The new status of this patch is: Ready for Committer
> Very cool.
>
> Thanks again for the review!

Thank you, Stephen
    / J.L.

Jose, all,

While reviewing and considering this patch further, I realized that
a pg_upgrade correctly restored the privileges of extension objects, but
it didn't copy through the pg_init_privs catalog entries for those
extension objects, meaning that a further pg_dump wouldn't realize that
the extension objects had any initial privileges.

After looking at a few options, I settled on taking an approach similar
to how extension dependencies are handled and created a backend function
to help out with that issue.  The backend function, only callable during
binary upgrade, sets a flag to let the system know that the GRANTs and
REVOKEs about to be executed need to be recorded into pg_init_privs,
just like they are done during a CREATE EXTENSION script.  pg_dump now
pulls out, during a binary upgrade for extension objects, what the delta
is between the object's default privileges and what is recorded in
pg_init_privs and issues the necessary GRANT and REVOKE statements in a
block surrounded by setting/unsetting of the flag.

That turned out to simplify the regular pg_dump extraction process for
ACLs as that part could then simply always assume that the objects in
the system start life with the privileges recorded in pg_init_privs (as
they always were for initdb'd objects but now, by the time we reach the
point where we are issuing the GRANTs and REVOKEs for the extension
objects, those are as well).

With all of these changes to the ACL extension bits of the queries, I
pulled those out into a single buildACLQueries() function instead of
repeating the more-or-less-the-same query fragments inside of every
get*() routine in pg_dump.  That refactoring really cleaned up the
pg_dump changes from that patch, in my view.

I also rebased the patch and dealt with the fallout from other recent
changes which have gone into the tree.

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.

Thanks!

Stephen

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.

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.

Noah, all,

* Noah Misch (noah@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

On Wed, Apr 20, 2016 at 11:12:44AM -0400, Stephen Frost wrote:
> * Noah Misch (noah@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?

On Wed, Apr 20, 2016 at 11:12:44AM -0400, Stephen Frost wrote:
> * Noah Misch (noah@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?

On Wed, Apr 20, 2016 at 10:50:21PM -0400, Stephen Frost wrote:
> On Wednesday, April 20, 2016, Noah Misch <noah@leadboat.com> 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/1718942738eb65c8407fcd864883f4c8@fuzzy.cz

On Fri, Apr 22, 2016 at 12:25 AM, Noah Misch <noah@leadboat.com> 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

* Noah Misch (noah@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

Noah, all,

* Noah Misch (noah@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

On Fri, Apr 22, 2016 at 12:31:41PM -0400, Stephen Frost wrote:
> * Noah Misch (noah@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.

On Fri, Apr 22, 2016 at 08:24:53PM -0400, Stephen Frost wrote:
> * Noah Misch (noah@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.

Noah, all,

* Noah Misch (noah@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 (noah@leadboat.com) wrote:
> On Fri, Apr 22, 2016 at 08:24:53PM -0400, Stephen Frost wrote:
> > * Noah Misch (noah@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 weekend.

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:

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

I'm planning to work on adding in other object types and, once that's
more-or-less complete, adding in a test for extensions and then adding
tests for GRANT/REVOKE on from-initdb and in-extension objects.

Thoughts?

Thanks!

Stephen

On Mon, Apr 25, 2016 at 12:39:09AM -0400, Stephen Frost wrote:
> * Noah Misch (noah@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.

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.

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.

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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.

On Fri, Apr 22, 2016 at 3:30 AM, Peter Geoghegan <pg@heroku.com> wrote:
> On Fri, Apr 22, 2016 at 12:25 AM, Noah Misch <noah@leadboat.com> 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

Noah,

* Noah Misch (noah@leadboat.com) wrote:
> On Mon, Apr 25, 2016 at 12:39:09AM -0400, Stephen Frost wrote:
> > * Noah Misch (noah@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

Noah, all,

* Stephen Frost (sfrost@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

On Wed, May 04, 2016 at 08:14:55AM -0400, Stephen Frost wrote:
> * Noah Misch (noah@leadboat.com) wrote:
> > On Mon, Apr 25, 2016 at 12:39:09AM -0400, Stephen Frost wrote:
> > > * Noah Misch (noah@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?)

* Noah Misch (noah@leadboat.com) wrote:
> On Wed, May 04, 2016 at 08:14:55AM -0400, Stephen Frost wrote:
> > * Noah Misch (noah@leadboat.com) wrote:
> > > On Mon, Apr 25, 2016 at 12:39:09AM -0400, Stephen Frost wrote:
> > > > * Noah Misch (noah@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

* Stephen Frost (sfrost@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

* Noah Misch (noah@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

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

* Alvaro Herrera (alvherre@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

Alvaro,

* Alvaro Herrera (alvherre@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