Review of GetUserId() Usage

Stephen Frost wrote:

pgstat_get_backend_current_activity()
Used to decide if the current activity string should be returned or
not. In my view, this is a clear case which should be addressed
through has_privs_of_role() instead of requiring the user to
SET ROLE to each role they are an inheirited member of to query for
what the other sessions are doing.

pg_signal_backend()
Used to decide if pg_terminate_backend and pg_cancel_backend are
allowed. Another case which should be changed over to
has_privs_of_role(), in my view. Requiring the user to SET ROLE for
roles which they are an inheirited member of is confusing as it's
generally not required.

pg_stat_get_activity()
Used to decide if the state information should be shared.
My opinion is the same as above- use has_privs_of_role().
There are a number of other functions in pgstatfuncs.c with similar
issues (eg: pg_stat_get_backend_activity(),
pg_stat_get_backend_client_port(), and others).

I think the case for pgstat_get_backend_current_activity() and
pg_stat_get_activity and the other pgstatfuncs.c callers is easy to make
and seems acceptable to me; but I would leave pg_signal_backend out of
that discussion, because it has a potentially harmful side effect. By
requiring SET ROLE you add an extra layer of protection against
mistakes. (Hopefully, pg_signal_backend() is not a routine thing for
well-run systems, which means human intervention, and therefore the room
for error isn't insignificant.)

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

I think the case for pgstat_get_backend_current_activity() and
pg_stat_get_activity and the other pgstatfuncs.c callers is easy to make
and seems acceptable to me; but I would leave pg_signal_backend out of
that discussion, because it has a potentially harmful side effect. By
requiring SET ROLE you add an extra layer of protection against
mistakes. (Hopefully, pg_signal_backend() is not a routine thing for
well-run systems, which means human intervention, and therefore the room
for error isn't insignificant.)

While I certainly understand where you're coming from, I don't really
buy into it. Yes, cancelling a query (the only thing normal users can
do anyway- they can't terminate backends) could mean the loss of any
in-progress work, but it's not like 'rm' and I don't see that it needs
to require extra hoops for individuals to go through.

Thanks!

Stephen

On 9/24/14 4:58 PM, Stephen Frost wrote:

Alvaro,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

I think the case for pgstat_get_backend_current_activity() and
pg_stat_get_activity and the other pgstatfuncs.c callers is easy to make
and seems acceptable to me; but I would leave pg_signal_backend out of
that discussion, because it has a potentially harmful side effect. By
requiring SET ROLE you add an extra layer of protection against
mistakes. (Hopefully, pg_signal_backend() is not a routine thing for
well-run systems, which means human intervention, and therefore the room
for error isn't insignificant.)

While I certainly understand where you're coming from, I don't really
buy into it. Yes, cancelling a query (the only thing normal users can
do anyway- they can't terminate backends) could mean the loss of any
in-progress work, but it's not like 'rm' and I don't see that it needs
to require extra hoops for individuals to go through.

It would be weird if it were inconsistent: some things require role
membership, some things require SET ROLE. Try explaining that.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Peter Eisentraut (peter_e@gmx.net) wrote:

On 9/24/14 4:58 PM, Stephen Frost wrote:

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

I think the case for pgstat_get_backend_current_activity() and
pg_stat_get_activity and the other pgstatfuncs.c callers is easy to make
and seems acceptable to me; but I would leave pg_signal_backend out of
that discussion, because it has a potentially harmful side effect. By
requiring SET ROLE you add an extra layer of protection against
mistakes. (Hopefully, pg_signal_backend() is not a routine thing for
well-run systems, which means human intervention, and therefore the room
for error isn't insignificant.)

While I certainly understand where you're coming from, I don't really
buy into it. Yes, cancelling a query (the only thing normal users can
do anyway- they can't terminate backends) could mean the loss of any
in-progress work, but it's not like 'rm' and I don't see that it needs
to require extra hoops for individuals to go through.

It would be weird if it were inconsistent: some things require role
membership, some things require SET ROLE. Try explaining that.

I agree.. We already have that distinction, through inherit vs.
noinherit. I don't think it makes sense to have it also for individual
commands which we feel "might not be as safe". You could still go
delete all their data w/o a set role if you wanted...

Thanks,

Stephen

* Peter Eisentraut (peter_e@gmx.net) wrote:

On 9/24/14 4:58 PM, Stephen Frost wrote:

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

I think the case for pgstat_get_backend_current_activity() and
pg_stat_get_activity and the other pgstatfuncs.c callers is easy to make
and seems acceptable to me; but I would leave pg_signal_backend out of
that discussion, because it has a potentially harmful side effect. By
requiring SET ROLE you add an extra layer of protection against
mistakes. (Hopefully, pg_signal_backend() is not a routine thing for
well-run systems, which means human intervention, and therefore the room
for error isn't insignificant.)

While I certainly understand where you're coming from, I don't really
buy into it. Yes, cancelling a query (the only thing normal users can
do anyway- they can't terminate backends) could mean the loss of any
in-progress work, but it's not like 'rm' and I don't see that it needs
to require extra hoops for individuals to go through.

It would be weird if it were inconsistent: some things require role
membership, some things require SET ROLE. Try explaining that.

Agreed.

As a side-note, this change is included in the 'role attributes' patch.

Might be worth splitting out if there is interest in back-patching this,
but as it's a behavior change, my thinking was that it wouldn't make
sense to back-patch.

Thanks,

Stephen

On Thu, Oct 16, 2014 at 9:49 AM, Stephen Frost <sfrost@snowman.net> wrote:

As a side-note, this change is included in the 'role attributes' patch.

It's really important that we keep separate changes in separate
patches that are committed in separate commits. Otherwise, it gets
really confusing.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert,

On Thursday, October 16, 2014, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Oct 16, 2014 at 9:49 AM, Stephen Frost <sfrost@snowman.net
<javascript:;>> wrote:

As a side-note, this change is included in the 'role attributes' patch.

It's really important that we keep separate changes in separate
patches that are committed in separate commits. Otherwise, it gets
really confusing.

I can do that, but it overlaps with the MONITORING role attribute changes
also..

Thanks,

Stephen

On Thu, Oct 16, 2014 at 11:28 AM, Stephen Frost <sfrost@snowman.net> wrote:

On Thursday, October 16, 2014, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Oct 16, 2014 at 9:49 AM, Stephen Frost <sfrost@snowman.net> wrote:

As a side-note, this change is included in the 'role attributes' patch.

It's really important that we keep separate changes in separate
patches that are committed in separate commits. Otherwise, it gets
really confusing.

I can do that, but it overlaps with the MONITORING role attribute changes
also..

I'm not sure what your point is. Whether keeping changes separate is
easy or hard, and whether things overlap with multiple other things or
just one, we need to make the effort to do it.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Robert Haas (robertmhaas@gmail.com) wrote:

I'm not sure what your point is. Whether keeping changes separate is
easy or hard, and whether things overlap with multiple other things or
just one, we need to make the effort to do it.

What I was getting at is that the role attributes patch would need to
depend on these changes.. If the two are completely independent then
one would fail to apply cleanly when/if the other is committed, that's
all.

I'll break them into three pieces- superuser() cleanup, GetUserId() ->
has_privs_of_role(), and the additional-role-attributes patch will just
depend on the others.

Thanks,

Stephen

I'll break them into three pieces- superuser() cleanup, GetUserId() ->
has_privs_of_role(), and the additional-role-attributes patch will just
depend on the others.

The superuser() cleanup has already been submitted to the current
commitfest.

https://commitfest.postgresql.org/action/patch_view?id=1587

-Adam

--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

All,

I'll break them into three pieces- superuser() cleanup, GetUserId() ->
has_privs_of_role(), and the additional-role-attributes patch will just
depend on the others.

Attached is a patch for the GetUserId() -> has_privs_of_role() cleanup for
review.

-Adam

--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

All,

* Peter Eisentraut (peter_e@gmx.net) wrote:

It would be weird if it were inconsistent: some things require role
membership, some things require SET ROLE. Try explaining that.

Attached is a patch to address the pg_cancel/terminate_backend and the
statistics info as discussed previously. It sounds like we're coming to
consensus that this is the correct approach. Comments are always
welcome, of course, but it's a pretty minor patch and I'd like to move
forward with it soon against master.

We'll rebase the role attributes patch with these changes and update
that patch for review (with a few other changes) after.

Thanks!

Stephen

* Stephen Frost (sfrost@snowman.net) wrote:

Attached is a patch to address the pg_cancel/terminate_backend and the
statistics info as discussed previously. It sounds like we're coming to

And I forgot the attachment, of course. Apologies.

Thanks,

Stephen

All,

* Stephen Frost (sfrost@snowman.net) wrote:

* Stephen Frost (sfrost@snowman.net) wrote:

Attached is a patch to address the pg_cancel/terminate_backend and the
statistics info as discussed previously. It sounds like we're coming to

And I forgot the attachment, of course. Apologies.

Updated patch attached which also changes the error messages (which
hadn't been updated in the prior versions and really should be).

Barring objections, I plan to move forward with this one and get this
relatively minor change wrapped up. As mentioned in the commit, while
this might be an arguably back-patchable change, the lack of field
complaints and the fact that it changes existing behavior mean it should
go only against master, imv.

Thanks,

Stephen

On Sat, Nov 29, 2014 at 3:00 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Stephen Frost (sfrost@snowman.net) wrote:

* Stephen Frost (sfrost@snowman.net) wrote:

Attached is a patch to address the pg_cancel/terminate_backend and the
statistics info as discussed previously. It sounds like we're coming to

And I forgot the attachment, of course. Apologies.

Updated patch attached which also changes the error messages (which
hadn't been updated in the prior versions and really should be).

Barring objections, I plan to move forward with this one and get this
relatively minor change wrapped up. As mentioned in the commit, while
this might be an arguably back-patchable change, the lack of field
complaints and the fact that it changes existing behavior mean it should
go only against master, imv.

This patch does a couple of different things:

1. It makes more of the crappy error message change that Andres and I
already objected to on the other thread. Whether you disagree with
those objections or not, don't make an end-run around them by putting
more of the same stuff into patches on other threads.

2. It changes the functions in pgstatfuncs.c so that you can see the
relevant information not only for your own role, but also for roles of
which you are a member. That seems fine, but do we need a
documentation change someplace?

3. It messes around with pg_signal_backend(). There are currently no
cases in which pg_signal_backend() throws an error, which is good,
because it lets you write queries against pg_stat_activity() that
don't fail halfway through, even if you are missing permissions on
some things. This patch introduces such a case, which is bad.

I think it's unfathomable that you would consider anything in this
patch a back-patchable bug fix. It's clearly a straight-up behavior
change... or more properly three different changes, only one of which
I agree with.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Sat, Nov 29, 2014 at 3:00 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Stephen Frost (sfrost@snowman.net) wrote:
Updated patch attached which also changes the error messages (which
hadn't been updated in the prior versions and really should be).

Barring objections, I plan to move forward with this one and get this
relatively minor change wrapped up. As mentioned in the commit, while
this might be an arguably back-patchable change, the lack of field
complaints and the fact that it changes existing behavior mean it should
go only against master, imv.

This patch does a couple of different things:

1. It makes more of the crappy error message change that Andres and I
already objected to on the other thread. Whether you disagree with
those objections or not, don't make an end-run around them by putting
more of the same stuff into patches on other threads.

The error message clearly needed to be updated either way or I wouldn't
have touched it. I changed it to match what I feel is the prevelant and
certainly more commonly seen messaging from PG when it comes to
permissions errors, and drew attention to it by commenting on the fact
that I changed it. Doing otherwise would have drawn similar criticism
(is it did upthread, by Peter or Alvaro, I believe..) that I wasn't
updating it to match the messaging which we should be using.

2. It changes the functions in pgstatfuncs.c so that you can see the
relevant information not only for your own role, but also for roles of
which you are a member. That seems fine, but do we need a
documentation change someplace?

Yes, I've added the documentation changes to my branch, just hadn't
posted an update yet (travelling today).

3. It messes around with pg_signal_backend(). There are currently no
cases in which pg_signal_backend() throws an error, which is good,
because it lets you write queries against pg_stat_activity() that
don't fail halfway through, even if you are missing permissions on
some things. This patch introduces such a case, which is bad.

Good point, I'll move that check up into the other functions, which will
allow for a more descriptive error as well.

I think it's unfathomable that you would consider anything in this
patch a back-patchable bug fix. It's clearly a straight-up behavior
change... or more properly three different changes, only one of which
I agree with.

I didn't think it was back-patchable and stated as much. I anticipated
that argument and provided my thoughts on it. I *do* think it's wrong
to be using GetUserId() in this case and it's only very slightly
mollified by being documented that way.

Thanks,

Stephen

* Stephen Frost (sfrost@snowman.net) wrote:

3. It messes around with pg_signal_backend(). There are currently no
cases in which pg_signal_backend() throws an error, which is good,
because it lets you write queries against pg_stat_activity() that
don't fail halfway through, even if you are missing permissions on
some things. This patch introduces such a case, which is bad.

Good point, I'll move that check up into the other functions, which will
allow for a more descriptive error as well.

Err, I'm missing something here, as pg_signal_backend() is a misc.c
static internal function? How would you be calling it from a query
against pg_stat_activity()?

I'm fine making the change anyway, just curious..

Thanks,

Stephen

On Tue, Dec 2, 2014 at 2:50 PM, Stephen Frost <sfrost@snowman.net> wrote:

1. It makes more of the crappy error message change that Andres and I
already objected to on the other thread. Whether you disagree with
those objections or not, don't make an end-run around them by putting
more of the same stuff into patches on other threads.

The error message clearly needed to be updated either way or I wouldn't
have touched it. I changed it to match what I feel is the prevelant and
certainly more commonly seen messaging from PG when it comes to
permissions errors, and drew attention to it by commenting on the fact
that I changed it. Doing otherwise would have drawn similar criticism
(is it did upthread, by Peter or Alvaro, I believe..) that I wasn't
updating it to match the messaging which we should be using.

OK, I guess that's a fair point.

I think it's unfathomable that you would consider anything in this
patch a back-patchable bug fix. It's clearly a straight-up behavior
change... or more properly three different changes, only one of which
I agree with.

I didn't think it was back-patchable and stated as much. I anticipated
that argument and provided my thoughts on it. I *do* think it's wrong
to be using GetUserId() in this case and it's only very slightly
mollified by being documented that way.

It's not wrong. It's just different than what you happen to prefer.
It's fine to want to change things, but "not the way I would have done
it" is not the same as "arguably a bug".

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Stephen Frost (sfrost@snowman.net) wrote:

* Stephen Frost (sfrost@snowman.net) wrote:

3. It messes around with pg_signal_backend(). There are currently no
cases in which pg_signal_backend() throws an error, which is good,
because it lets you write queries against pg_stat_activity() that
don't fail halfway through, even if you are missing permissions on
some things. This patch introduces such a case, which is bad.

Good point, I'll move that check up into the other functions, which will
allow for a more descriptive error as well.

Err, I'm missing something here, as pg_signal_backend() is a misc.c
static internal function? How would you be calling it from a query
against pg_stat_activity()?

I'm fine making the change anyway, just curious..

Updated patch attached which move the ereport() out of
pg_signal_backend() and into its callers, as the other permissions
checks are done, and includes the documentation changes. The error
messages are minimally changed to match the new behvaior.

Thanks,

Stephen