Обсуждение: Default Roles (was: Additional role attributes)

All,

Starting a new thread, as suggested by Robert, for consideration of
adding default roles for sets of administrative functions, therefore
removing the need for superuser-level roles in many use-cases.

This reserves the prefix 'pg_' as being for default roles.

Having these default roles also means that third party applications
(eg: check_postgres.pl) can depend on their availability in their
installation instructions and have a clear understanding of what they
provide.

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Wed, Apr 29, 2015 at 8:20 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > On this part I have a bit of a problem -- the prefix is not really
> > reserved, is it.  I mean, evidently it's still possible to create roles
> > with the pg_ prefix ... otherwise, how come the new lines to
> > system_views.sql that create the "predefined" roles work in the first
> > place?  I think if we're going to reserve role names, we should reserve
> > them for real: CREATE ROLE should flat out reject creation of such
> > roles, and the default ones should be created during bootstrap.

Agreed, and done.

> This is exactly what I mean about needing separate discussion for
> separate parts of the patch.  There's so much different stuff in there
> right now that objections like this won't necessarily come out until
> it's far too late to change things around.

This is part 2 and really the "guts" of the changes proposed.  Part 1
was the patch sent earlier today to change pg_stat_get_activity() to use
a tuplestore, and this patch depends on that one.  I'll rebase and
resend after that's gone in.  I did notice that Andres just pushed
upsert though, and it wouldn't surprise me if there are now merge
conflicts.  Will address all of that tomorrow, in any case.

This patch is significantly reduced in complexity (it's literally less
than 1/3 of the prior patch, with the largest change being in
system_views.sql where all the REVOKE/GRANT commands are dropped) as it
doesn't modify pg_dump, at all.  pg_dumpall is minimally modified by
simply not dumping out roles starting with "pg_" on 9.5 and above
systems.  pg_upgrade is similairly modified to check for roles which
start with "pg_" in pre-9.5 versions and complain if found.

I'll be playing around with the patch itself, testing, etc, but what we
really need is a discussion on if anyone is concerned about reserving
"pg_" for default roles.  Exactly what roles are created and what
privileges are granted to them can be tweaked easily, though there has
been little discussion and therefore, presumably, little issue with the
categories that were proposed back in October when this was proposed
with role attributes instead of default roles.

As for the previously proposed changes to pg_dump, I don't particularly
have a reason to continue with that effort unless others are interested.
The default roles proposed in this patch solve the use-cases which I had
set out to solve 6 months ago.  Please let me know if there is interest
in changing pg_dump to try and preserve permissions which are set in
pg_catalog by administrators, but we've never supported that and it
might be best left as-is.

    Thanks!

        Stephen

All,

* Stephen Frost (sfrost@snowman.net) wrote:
> Starting a new thread, as suggested by Robert, for consideration of
> adding default roles for sets of administrative functions, therefore
> removing the need for superuser-level roles in many use-cases.

[...]

> This is part 2 and really the "guts" of the changes proposed.  Part 1
> was the patch sent earlier today to change pg_stat_get_activity() to use
> a tuplestore, and this patch depends on that one.  I'll rebase and
> resend after that's gone in.  I did notice that Andres just pushed
> upsert though, and it wouldn't surprise me if there are now merge
> conflicts.  Will address all of that tomorrow, in any case.

Here's a rebase with a few additional items as follows.

Andres suggested that we drop the REPLICATION role attribute and just
use membership in pg_replication instead.  That's turned out quite
fantastically as we can now handle upgrades without breaking anything-
CREATE ROLE and ALTER ROLE still accept the attribute but simply grant
pg_replication to the role instead, and postinit.c has been changed to
check role membership similar to other pg_hba role membership checks
when a replication connection comes in.  Hat's off to Andres for his
suggestion.

I've added two more default roles, since it was pointed out to me that I
hadn't exactly mimicked the role attributes originally proposed.  These
are pg_rotate_logfile and pg_signal_backend.  This also removes any
direct object grants to pg_admin; it now means only "all of the other
roles combined" without anything additional.

Documentation and regression tests updated.

Comments and suggestions are most welcome, as always.

    Thanks!

        Stephen

All,

This patch gets smaller and smaller.

Upon reflection I realized that, with default roles, it's entirely
unnecssary to change how the permission checks happen today- we can
simply add checks to them to be looking at role membership also.  That's
removed the last of my concerns regarding any API breakage for existing
use-cases and has greatly simplified things overall.

This does change the XLOG functions to require pg_monitor, as discussed
on the other thread where it was pointed out by Heikki that the XLOG
location information could be used to extract sensitive information
based on what happens during compression.  Adding docs explaining that
is on my to-do list for tomorrow.

* Stephen Frost (sfrost@snowman.net) wrote:
> Andres suggested that we drop the REPLICATION role attribute and just
> use membership in pg_replication instead.  That's turned out quite
> fantastically as we can now handle upgrades without breaking anything-
> CREATE ROLE and ALTER ROLE still accept the attribute but simply grant
> pg_replication to the role instead, and postinit.c has been changed to
> check role membership similar to other pg_hba role membership checks
> when a replication connection comes in.  Hat's off to Andres for his
> suggestion.

It's also unnecessary to change how the REPLICATION role attribute
functions today.  This patch does add the pg_replication role, but it's
only allowed to execute the various pg_logical and friends functions and
not to actually connect as a REPLICATION user.  Connecting as a
REPLICATION user allows you to stream the entire contents of the
backend, after all, so it makes sense to have that be independent.

I added another default role which allows the user to view
pg_show_file_settings, as that seemed useful to me.  The diffstat for
that being something like 4 additions without docs and maybe 10 with.
More documentation would probably be good though and I'll look at adding
to it.

Most of the rest of what I've done has simply been reverting back to
what we had.  The patch is certainly far easier to verify by reading
through it now, as the changes are right next to each other, and the
regression output changes are much smaller.

Thoughts?  Comments?  Suggestions?

    Thanks!

        Stephen

On 05/13/2015 06:07 AM, Stephen Frost wrote:
> This does change the XLOG functions to require pg_monitor, as discussed
> on the other thread where it was pointed out by Heikki that the XLOG
> location information could be used to extract sensitive information
> based on what happens during compression.

That seems like an orthogonal issue, not something that should be 
bundled in this patch. IIRC we didn't reach a consensus on what to do 
about the compression-leaks-information issue. One idea was to make it 
configurable on a per-table basis, and if we do that, perhaps we don't 
need to restrict access to pg_current_xlog_location() and friends.

- Heikki

* Heikki Linnakangas (hlinnaka@iki.fi) wrote:
> On 05/13/2015 06:07 AM, Stephen Frost wrote:
> >This does change the XLOG functions to require pg_monitor, as discussed
> >on the other thread where it was pointed out by Heikki that the XLOG
> >location information could be used to extract sensitive information
> >based on what happens during compression.
>
> That seems like an orthogonal issue, not something that should be
> bundled in this patch. IIRC we didn't reach a consensus on what to
> do about the compression-leaks-information issue. One idea was to
> make it configurable on a per-table basis, and if we do that,
> perhaps we don't need to restrict access to
> pg_current_xlog_location() and friends.

Alright, I'll pull it out.  I see it's already been added to the
open-items list, so we shouldn't forget about it.

For my 2c, I'd much rather have the information restricted to a
privileged role instead of having to disable the feature.  Further, all
tables need to be considered as having privileged information, not just
systems ones like pg_authid, as the user might not have rights on the
other columns or rows in the table.
Thanks!
    Stephen

All,

* Heikki Linnakangas (hlinnaka@iki.fi) wrote:
> On 05/13/2015 06:07 AM, Stephen Frost wrote:
> >This does change the XLOG functions to require pg_monitor, as discussed
> >on the other thread where it was pointed out by Heikki that the XLOG
> >location information could be used to extract sensitive information
> >based on what happens during compression.
>
> That seems like an orthogonal issue, not something that should be
> bundled in this patch. IIRC we didn't reach a consensus on what to
> do about the compression-leaks-information issue. One idea was to
> make it configurable on a per-table basis, and if we do that,
> perhaps we don't need to restrict access to
> pg_current_xlog_location() and friends.

Updated patch attached which removes the changes to the XLOG location
functions and adds checks for AlterRole and RenameRole to prevent
altering or renaming the default roles.  Also adds '\duS'/'\dgS'
support to psql, to show default roles only when asked.

    Thanks!

        Stephen

On Tue, May 12, 2015 at 11:07 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Thoughts?  Comments?  Suggestions?

Yes: let's punt this to 9.6.  The decisions you're making here are way
too significant to be making a couple of days before feature freeze,
and this patch has changed massively since it was first submitted.
There isn't time now for people who want to have an opinion on this to
form an educated one.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, May 13, 2015 at 10:16:39AM -0400, Robert Haas wrote:
> On Tue, May 12, 2015 at 11:07 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > Thoughts?  Comments?  Suggestions?
> 
> Yes: let's punt this to 9.6.  The decisions you're making here are way
> too significant to be making a couple of days before feature freeze,
> and this patch has changed massively since it was first submitted.
> There isn't time now for people who want to have an opinion on this to
> form an educated one.

Yeah, pretty much any patch that needs significant redesign at this
point should be kept for 9.6.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

* Robert Haas (robertmhaas@gmail.com) wrote:
> Yes: let's punt this to 9.6.  The decisions you're making here are way
> too significant to be making a couple of days before feature freeze,
> and this patch has changed massively since it was first submitted.
> There isn't time now for people who want to have an opinion on this to
> form an educated one.

Perhaps I'm missing something, but the patch has been simplified down to
the point where the only question seems to be "should we have default
roles or not?", which I had asked about two weeks ago and again last
week on a new thread.  I feel like we're waiting for the silent majority
to chime in.

Put another way, I'm afraid that posting this next week, next month, or
next year is going to garner just as many responses as it's seen in the
past 2 weeks, while I continue to field questions on -bugs, -admin, and
IRC about "how do I set up Nagios with a non-superuser account?" and
similar issues.  It's not a novel idea, certainly;  Magnus suggested it
back in December on the thread, Tom made a similar comment that it might
make sense to have them later on and it's come up quite a few times
previously as it's something other RDBMS's have and we don't.  Clearly,
others have read the proposal, at least (You and Alvaro on the other
thread, Heikki on this one).

It's my fault that I didn't follow up on their suggestions earlier and
instead spent a bunch of time fighting with pg_dump, but it doesn't seem
like there is a lot of disagreement about the idea.  I'd offer to
simplify it down, but it seems like the obvious change in that direction
would be to reserve pg_ as a role prefix and not actually create any
default roles, but that doesn't gain us anything and makes a potential
headache for users without any feature to go with it.

Bruce's point is a better one, except that all of the changes have been
about reducing changes to core, down to an almost trivial level.  I wish
it had been a smoother ride to get here from the original proposal six
months ago, but I've certainly got a better understanding of the level
of effort involved and changes required for the other approaches and
this certainly seems like the best and simplest.
Thanks!
    Stephen

On Wed, May 13, 2015 at 11:50 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Robert Haas (robertmhaas@gmail.com) wrote:
>> Yes: let's punt this to 9.6.  The decisions you're making here are way
>> too significant to be making a couple of days before feature freeze,
>> and this patch has changed massively since it was first submitted.
>> There isn't time now for people who want to have an opinion on this to
>> form an educated one.
>
> Perhaps I'm missing something, but the patch has been simplified down to
> the point where the only question seems to be "should we have default
> roles or not?", which I had asked about two weeks ago and again last
> week on a new thread.  I feel like we're waiting for the silent majority
> to chime in.

The thing is, right now, there are many, many patches in flight and
everybody is really, really busy with them.  What we should be trying
to push in right now are the patches that we know we want, and there
are at most a few minor implementation details to sort out.  We should
not be trying to push in any patches where we are not confident in the
design.  I really don't see how you can be confident that this design
will have the backing of the community at this point.  It's changed in
major ways, multiple times.  The latest version, again majorly
revised, was posted TWO DAYS before feature freeze.  Two days is not
enough time to get meaningful feedback on significant design decisions
under the best of circumstances, and even less so when those two days
are the last remaining days before feature freeze.

Now, if six people who are all well-known PostgreSQL contributors show
up and they all say "I looked at the latest version of this carefully
and I'm confident you've got it right", then go ahead: push it.  But
don't make the mistake of thinking that because you're confident that
you've now got it right everybody else will like it too.  Even since
you posted the last version, Heikki expressed a concern that resulted
in (surprise!) more revisions.  There comes a point where a patch that
is still heavily in flux is just too late for the release cycle, and
we're well past that point at this stage of the game.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> Now, if six people who are all well-known PostgreSQL contributors show
> up and they all say "I looked at the latest version of this carefully
> and I'm confident you've got it right", then go ahead: push it.  But
> don't make the mistake of thinking that because you're confident that
> you've now got it right everybody else will like it too.  Even since
> you posted the last version, Heikki expressed a concern that resulted
> in (surprise!) more revisions.  There comes a point where a patch that
> is still heavily in flux is just too late for the release cycle, and
> we're well past that point at this stage of the game.

FWIW, I agree that we're past the point where we should be committing
features whose external definition hasn't been stable for awhile.  Fixing
bugs post-feature-freeze is one thing, but if there's a significant chance
that you'll be having to adjust the feature definition, then it's probably
too late for 9.5.  And this particular item sure looks like it's in that
category.

There's always another release cycle.
        regards, tom lane

On Wed, May 13, 2015 at 12:07 PM, Stephen Frost <sfrost@snowman.net> wrote:
> All,
>
> This patch gets smaller and smaller.
>
> Upon reflection I realized that, with default roles, it's entirely
> unnecssary to change how the permission checks happen today- we can
> simply add checks to them to be looking at role membership also.  That's
> removed the last of my concerns regarding any API breakage for existing
> use-cases and has greatly simplified things overall.
>
> This does change the XLOG functions to require pg_monitor, as discussed
> on the other thread where it was pointed out by Heikki that the XLOG
> location information could be used to extract sensitive information
> based on what happens during compression.  Adding docs explaining that
> is on my to-do list for tomorrow.
>
> * Stephen Frost (sfrost@snowman.net) wrote:
>> Andres suggested that we drop the REPLICATION role attribute and just
>> use membership in pg_replication instead.  That's turned out quite
>> fantastically as we can now handle upgrades without breaking anything-
>> CREATE ROLE and ALTER ROLE still accept the attribute but simply grant
>> pg_replication to the role instead, and postinit.c has been changed to
>> check role membership similar to other pg_hba role membership checks
>> when a replication connection comes in.  Hat's off to Andres for his
>> suggestion.
>
> It's also unnecessary to change how the REPLICATION role attribute
> functions today.  This patch does add the pg_replication role, but it's
> only allowed to execute the various pg_logical and friends functions and
> not to actually connect as a REPLICATION user.  Connecting as a
> REPLICATION user allows you to stream the entire contents of the
> backend, after all, so it makes sense to have that be independent.
>
> I added another default role which allows the user to view
> pg_show_file_settings, as that seemed useful to me.  The diffstat for
> that being something like 4 additions without docs and maybe 10 with.
> More documentation would probably be good though and I'll look at adding
> to it.
>
> Most of the rest of what I've done has simply been reverting back to
> what we had.  The patch is certainly far easier to verify by reading
> through it now, as the changes are right next to each other, and the
> regression output changes are much smaller.
>
> Thoughts?  Comments?  Suggestions?

he documents of the functions which the corresponding default roles
are added by this patch need to be updated. For example, the description
of pg_xlog_replay_pause() says "Pauses recovery immediately (restricted
to superusers).". I think that the description needs to mention
the corresponding default role "pg_replay". Otherwise, it's difficult for
users to see which default role is related to the function they want to use.
Or probably we can add the table explaining all the relationships between
default roles and corresponding operations. And it's useful.

Why do we allow users to change the attributes of default roles?
For example, ALTER ROLE default_role or GRANT ... TO default_role.
Those changes are not dumped by pg_dumpall. So if users change
the attributes for some reasons but they disappear via pg_dumpall,
maybe the system goes into unexpected state.

I think that it's better to allow the roles with pg_monitor to
execute pgstattuple functions. They are usually used for monitoring.
Thought?

Regards,

-- 
Fujii Masao

Fujii,

* Fujii Masao (masao.fujii@gmail.com) wrote:
> he documents of the functions which the corresponding default roles
> are added by this patch need to be updated. For example, the description
> of pg_xlog_replay_pause() says "Pauses recovery immediately (restricted
> to superusers).". I think that the description needs to mention
> the corresponding default role "pg_replay". Otherwise, it's difficult for
> users to see which default role is related to the function they want to use.
> Or probably we can add the table explaining all the relationships between
> default roles and corresponding operations. And it's useful.

Certainly, totally agree that we need to make it clear in the function
descriptions also.

> Why do we allow users to change the attributes of default roles?
> For example, ALTER ROLE default_role or GRANT ... TO default_role.
> Those changes are not dumped by pg_dumpall. So if users change
> the attributes for some reasons but they disappear via pg_dumpall,
> maybe the system goes into unexpected state.

Good point.  I'm fine with simply disallowing that completely; does
anyone want to argue that we should allow superusers to ALTER or GRANT
to these roles?  I have a hard time seeing the need for that and it
could make things quite ugly.

> I think that it's better to allow the roles with pg_monitor to
> execute pgstattuple functions. They are usually used for monitoring.
> Thought?

Possibly, but I'd need to look at them more closely than I have time to
right now.  Can you provide a use-case?  That would certainly help.
Also, we are mostly focused on things which are currently superuser-only
capabilities, if you don't need to be superuser today then the
monitoring system could be granted access using the normal mechanisms.
Actually logging systems won't log in directly as "pg_monitor" anyway,
they'll log in as "nagios" or similar, which has been GRANT'd
"pg_monitor" and could certainly be GRANT'd other rights also.
Thanks!
    Stephen

On Tue, Jul 14, 2015 at 3:46 AM, Stephen Frost <sfrost@snowman.net> wrote:
> Fujii,
>
> * Fujii Masao (masao.fujii@gmail.com) wrote:
>> he documents of the functions which the corresponding default roles
>> are added by this patch need to be updated. For example, the description
>> of pg_xlog_replay_pause() says "Pauses recovery immediately (restricted
>> to superusers).". I think that the description needs to mention
>> the corresponding default role "pg_replay". Otherwise, it's difficult for
>> users to see which default role is related to the function they want to use.
>> Or probably we can add the table explaining all the relationships between
>> default roles and corresponding operations. And it's useful.
>
> Certainly, totally agree that we need to make it clear in the function
> descriptions also.
>
>> Why do we allow users to change the attributes of default roles?
>> For example, ALTER ROLE default_role or GRANT ... TO default_role.
>> Those changes are not dumped by pg_dumpall. So if users change
>> the attributes for some reasons but they disappear via pg_dumpall,
>> maybe the system goes into unexpected state.
>
> Good point.  I'm fine with simply disallowing that completely; does
> anyone want to argue that we should allow superusers to ALTER or GRANT
> to these roles?  I have a hard time seeing the need for that and it
> could make things quite ugly.
>
>> I think that it's better to allow the roles with pg_monitor to
>> execute pgstattuple functions. They are usually used for monitoring.
>> Thought?
>
> Possibly, but I'd need to look at them more closely than I have time to
> right now.  Can you provide a use-case?  That would certainly help.

I have seen the monitoring system which periodically executes
the statement like
   SELECT * FROM pgstattuple('hoge');

to monitor the relation's physical length, the number of dead
tuples, etc. Then, for example, if the number of dead tuples is
increased unexpectedly and the relation becomes bloated, DBA tries
to find out the cause and execute the maintenance commands
if necessary to alleviate the situation. The monitoring system calls
pgstattuple() at off-peak times because pgstattuple() needs to
scan all the pages in the relation and its performance penalty
might be big.

> Also, we are mostly focused on things which are currently superuser-only
> capabilities, if you don't need to be superuser today then the
> monitoring system could be granted access using the normal mechanisms.

Currently only superusers can call pgstattuple().

Regards,

-- 
Fujii Masao

Stephen,

On Tue, Jul 14, 2015 at 9:22 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Tue, Jul 14, 2015 at 3:46 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > Fujii,
> >
> > * Fujii Masao (masao.fujii@gmail.com) wrote:
> >> he documents of the functions which the corresponding default roles
> >> are added by this patch need to be updated. For example, the description
> >> of pg_xlog_replay_pause() says "Pauses recovery immediately (restricted
> >> to superusers).". I think that the description needs to mention
> >> the corresponding default role "pg_replay". Otherwise, it's difficult for
> >> users to see which default role is related to the function they want to use.
> >> Or probably we can add the table explaining all the relationships between
> >> default roles and corresponding operations. And it's useful.
> >
> > Certainly, totally agree that we need to make it clear in the function
> > descriptions also.
> >
> >> Why do we allow users to change the attributes of default roles?
> >> For example, ALTER ROLE default_role or GRANT ... TO default_role.
> >> Those changes are not dumped by pg_dumpall. So if users change
> >> the attributes for some reasons but they disappear via pg_dumpall,
> >> maybe the system goes into unexpected state.
> >
> > Good point.  I'm fine with simply disallowing that completely; does
> > anyone want to argue that we should allow superusers to ALTER or GRANT
> > to these roles?  I have a hard time seeing the need for that and it
> > could make things quite ugly.
> >
> >> I think that it's better to allow the roles with pg_monitor to
> >> execute pgstattuple functions. They are usually used for monitoring.
> >> Thought?
> >
> > Possibly, but I'd need to look at them more closely than I have time to
> > right now.  Can you provide a use-case?  That would certainly help.
>
> I have seen the monitoring system which periodically executes
> the statement like
>
>     SELECT * FROM pgstattuple('hoge');
>
> to monitor the relation's physical length, the number of dead
> tuples, etc. Then, for example, if the number of dead tuples is
> increased unexpectedly and the relation becomes bloated, DBA tries
> to find out the cause and execute the maintenance commands
> if necessary to alleviate the situation. The monitoring system calls
> pgstattuple() at off-peak times because pgstattuple() needs to
> scan all the pages in the relation and its performance penalty
> might be big.
>
> > Also, we are mostly focused on things which are currently superuser-only
> > capabilities, if you don't need to be superuser today then the
> > monitoring system could be granted access using the normal mechanisms.
>
> Currently only superusers can call pgstattuple().


Will there be any work on this patch for this commit fest or not? This
is being carried around for a couple of months now with not much
progress. This thread is idle for 4 months now.
-- 
Michael

Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:
> Will there be any work on this patch for this commit fest or not? This
> is being carried around for a couple of months now with not much
> progress. This thread is idle for 4 months now.

This thread and the other one kind of merged.  The last update on the
overall discussion is here:

http://www.postgresql.org/message-id/20150930111120.GM3685@tamriel.snowman.net

Which was closer to 1.5 months ago and was the requested split of the
patch, which mainly needs to get review and/or buy-in from others on the
reservation of the role prefix 'pg_' (which is the first patch).

I'm happy to update the patches if they don't apply, of course, but
they're relatively straight-forward and we just need to agree that
reservation of the prefix is acceptable and then I can just commit
them.

Thanks!

Stephen

On Wed, Nov 18, 2015 at 5:36 AM, Stephen Frost <sfrost@snowman.net> wrote:
> Michael,
>
> * Michael Paquier (michael.paquier@gmail.com) wrote:
>> Will there be any work on this patch for this commit fest or not? This
>> is being carried around for a couple of months now with not much
>> progress. This thread is idle for 4 months now.
>
> This thread and the other one kind of merged.  The last update on the
> overall discussion is here:
>
> http://www.postgresql.org/message-id/20150930111120.GM3685@tamriel.snowman.net

Right. The CF entry links to two threads, and I somehow missed the first one.

> Which was closer to 1.5 months ago and was the requested split of the
> patch, which mainly needs to get review and/or buy-in from others on the
> reservation of the role prefix 'pg_' (which is the first patch).
> I'm happy to update the patches if they don't apply, of course, but
> they're relatively straight-forward and we just need to agree that
> reservation of the prefix is acceptable and then I can just commit
> them.

I'll reply directly on the other thread, there is no meaning to mess
up things here.
-- 
Michael

Fujii,

* Fujii Masao (masao.fujii@gmail.com) wrote:
> On Tue, Jul 14, 2015 at 3:46 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > Possibly, but I'd need to look at them more closely than I have time to
> > right now.  Can you provide a use-case?  That would certainly help.
>
> I have seen the monitoring system which periodically executes
> the statement like
>
>     SELECT * FROM pgstattuple('hoge');
>
> to monitor the relation's physical length, the number of dead
> tuples, etc. Then, for example, if the number of dead tuples is
> increased unexpectedly and the relation becomes bloated, DBA tries
> to find out the cause and execute the maintenance commands
> if necessary to alleviate the situation. The monitoring system calls
> pgstattuple() at off-peak times because pgstattuple() needs to
> scan all the pages in the relation and its performance penalty
> might be big.
[...]
> Currently only superusers can call pgstattuple().

I started looking into this.

If we were starting from a green field, the pg_dump dump catalog ACLs
patch would work just fine for this case.  Simply remove the superuser
checks and REVOKE EXECUTE from public in the script and we're done.

Unfortunately, we aren't, and that's where things get complicated.  The
usual pg_upgrade case will, quite correctly, dump out the objects
exactly as they exist from the 9.5-or-earlier system and restore them
into the 9.6 system, however, the new .so will be installed and that .so
won't have the superuser checks in it.

The only approach to addressing this which I can think of offhand would
be to have the new .so library check the version of the extension and,
for the 1.3 (pre-9.6) and previous versions, keep the superuser check,
but skip it for 1.4 (9.6) and later versions.

I'm certainly open to other suggestions, of course.  Would be great to
remove those superuser() checks and allow non-superusers to be GRANT'd
the right to run those functions, as discussed.

Thanks!

Stephen

On Sun, Apr 03, 2016 at 10:27:02PM -0400, Stephen Frost wrote:
> * Fujii Masao (masao.fujii@gmail.com) wrote:
> > Currently only superusers can call pgstattuple().
> 
> I started looking into this.
> 
> If we were starting from a green field, the pg_dump dump catalog ACLs
> patch would work just fine for this case.  Simply remove the superuser
> checks and REVOKE EXECUTE from public in the script and we're done.
> 
> Unfortunately, we aren't, and that's where things get complicated.  The
> usual pg_upgrade case will, quite correctly, dump out the objects
> exactly as they exist from the 9.5-or-earlier system and restore them
> into the 9.6 system, however, the new .so will be installed and that .so
> won't have the superuser checks in it.
> 
> The only approach to addressing this which I can think of offhand would
> be to have the new .so library check the version of the extension and,
> for the 1.3 (pre-9.6) and previous versions, keep the superuser check,
> but skip it for 1.4 (9.6) and later versions.

At the C level, have a pgstattuple function and a pgstattuple_v1_4 function.
Let them differ only in that the former has a superuser check.  Binary
upgrades will use the former, and fresh CREATE EXTENSION shall use the latter.

Noah,

* Noah Misch (noah@leadboat.com) wrote:
> On Sun, Apr 03, 2016 at 10:27:02PM -0400, Stephen Frost wrote:
> > * Fujii Masao (masao.fujii@gmail.com) wrote:
> > > Currently only superusers can call pgstattuple().
> >
> > I started looking into this.
> >
> > If we were starting from a green field, the pg_dump dump catalog ACLs
> > patch would work just fine for this case.  Simply remove the superuser
> > checks and REVOKE EXECUTE from public in the script and we're done.
> >
> > Unfortunately, we aren't, and that's where things get complicated.  The
> > usual pg_upgrade case will, quite correctly, dump out the objects
> > exactly as they exist from the 9.5-or-earlier system and restore them
> > into the 9.6 system, however, the new .so will be installed and that .so
> > won't have the superuser checks in it.
> >
> > The only approach to addressing this which I can think of offhand would
> > be to have the new .so library check the version of the extension and,
> > for the 1.3 (pre-9.6) and previous versions, keep the superuser check,
> > but skip it for 1.4 (9.6) and later versions.
>
> At the C level, have a pgstattuple function and a pgstattuple_v1_4 function.
> Let them differ only in that the former has a superuser check.  Binary
> upgrades will use the former, and fresh CREATE EXTENSION shall use the latter.

Excellent suggestion and many thanks for that.

I'll draft up a patch for that.

Thanks again!

Stephen

Noah, Fujii, all,

* Noah Misch (noah@leadboat.com) wrote:
> At the C level, have a pgstattuple function and a pgstattuple_v1_4 function.
> Let them differ only in that the former has a superuser check.  Binary
> upgrades will use the former, and fresh CREATE EXTENSION shall use the latter.

Attached is a patch which implements this for the pgstattuple
extensions.  The changes are pretty straight-forward, but I'm not going
to commit this under the gun of the feature freeze without at least
another committer reviewing it or getting an extension for a couple days
to play with it further and convince myself it's safe.

Ultimately, I'd like for this to be included in 9.6 as it'd be an
example use-case for others to follow when updating their extensions to
make use of the new pg_dump features, but I certainly don't see it as
being critical to the release.

Fujii, my apologies for not getting this done earlier, I know this is a
capability you are looking forward to having.

Thanks!

Stephen