Re: pg_dump dump catalog ACLs

Tom, all,

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