Re: pg_dump dump catalog ACLs

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