Re: pg_dump dump catalog ACLs

Alexander,

* Alexander Korotkov (a(dot)korotkov(at)postgrespro(dot)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(at)snowman(dot)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