Re: Additional role attributes && superuser review

Robert,

* Stephen Frost (sfrost@snowman.net) wrote:
> * Robert Haas (robertmhaas@gmail.com) wrote:
> > On Thu, Apr 2, 2015 at 12:53 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > > Clearly, further testing and documentation is required and I'll be
> > > getting to that over the next couple of days, but it's pretty darn late
> > > and I'm currently getting libpq undefined reference errors, probably
> > > because I need to set LD_LIBRARY_PATH, but I'm just too tired to now. :)
> > >
> > > Thoughts?
> >
> > The tricky part of this seems to me to be the pg_dump changes.  The
> > new catalog flag seems a little sketchy to me; wouldn't it be better
> > to make the dump flag into an enum, DUMP_EVERYTHING, DUMP_ACL,
> > DUMP_NONE?
>
> I agree that the pg_dump changes are a very big part of this change.
> I'll look at using an enum and see if that would work.

I've implemented this approach and there are things which I like about
it and things which I don't.  I'd love to hear your thoughts.  As
mentioned previously, this patch does not break the pg_stat_activity or
pg_stat_replication view APIs.  Similairly, the functions which directly
feed those views return the same results they did previously, there are
just additional functions now which provide everything, and view on top
of those, for users who are GRANT'd access to them.

This does change the API of a few functions which previously allowed
roles with the "replication" attribute to call them.  We could provide
additional functions to provide both paths but I don't believe it's
really necessary.  Indeed, I feel it's better for administrators to
explicit grant access to those functions instead.

Note that this doesn't use an enum but a bit field for which components
of a given object should be dumped out.  While I like that in general,
it meant a lot of changes and I'll be the first to admit that I've not
tested every possible pg_dump option permutation to make sure that the
correct results are returned.  I plan to do that in the coming weeks and
will address any issues found.

Is this, more-or-less, what you were thinking of?  I looked at removing
the relatively grotty options (dataOnly, aclsSkip, etc) and it didn't
appear trivial to use the bitmask instead.  I can look into that further
though, as I do feel that it'd be good if we could reduce our dependence
on those options in favor of the bitmask approach.

Thoughts?

    Thanks!

        Stephen