Re: pg_dump dump catalog ACLs - Mailing list pgsql-hackers

From Stephen Frost
Subject Re: pg_dump dump catalog ACLs
Date
Msg-id 20160314160057.GU3127@tamriel.snowman.net
Whole thread Raw
In response to Re: pg_dump dump catalog ACLs  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: pg_dump dump catalog ACLs
List pgsql-hackers
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

Attachment

pgsql-hackers by date:

Previous
From: Artur Zakirov
Date:
Subject: Re: plpgsql - DECLARE - cannot to use %TYPE or %ROWTYPE for composite types
Next
From: Tom Lane
Date:
Subject: Re: plpgsql - DECLARE - cannot to use %TYPE or %ROWTYPE for composite types