Re: [HACKERS] Review of GetUserId() Usage

2015-02-27 Thread Stephen Frost
Jeevan, * Jeevan Chalke (jeevan.cha...@gmail.com) wrote: The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested,

Re: [HACKERS] Review of GetUserId() Usage

2015-02-26 Thread Jeevan Chalke
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed I have reviewed the patch. Patch is excellent in shape and

Re: [HACKERS] Review of GetUserId() Usage

2015-02-13 Thread Michael Paquier
On Fri, Dec 5, 2014 at 11:28 PM, Stephen Frost sfr...@snowman.net wrote: * Stephen Frost (sfr...@snowman.net) wrote: * Stephen Frost (sfr...@snowman.net) wrote: 3. It messes around with pg_signal_backend(). There are currently no cases in which pg_signal_backend() throws an error,

Re: [HACKERS] Review of GetUserId() Usage

2014-12-05 Thread Stephen Frost
* Stephen Frost (sfr...@snowman.net) wrote: * Stephen Frost (sfr...@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()

Re: [HACKERS] Review of GetUserId() Usage

2014-12-05 Thread Andres Freund
On 2014-12-05 09:28:13 -0500, Stephen Frost wrote: static int pg_signal_backend(int pid, int sig) { @@ -113,7 +117,12 @@ pg_signal_backend(int pid, int sig) return SIGNAL_BACKEND_ERROR; } - if (!(superuser() || proc-roleId == GetUserId())) + /* Only allow

Re: [HACKERS] Review of GetUserId() Usage

2014-12-05 Thread Stephen Frost
* Andres Freund (and...@2ndquadrant.com) wrote: Is the 'Only allow superusers to signal superuser-owned backends' check actually safe that way? I personally try to never use a superuser role as the login user, but grant my account a superuser role that doesn't inherit. But IIRC PGPROC-roleId

Re: [HACKERS] Review of GetUserId() Usage

2014-12-02 Thread Robert Haas
On Sat, Nov 29, 2014 at 3:00 PM, Stephen Frost sfr...@snowman.net wrote: * Stephen Frost (sfr...@snowman.net) wrote: * Stephen Frost (sfr...@snowman.net) wrote: Attached is a patch to address the pg_cancel/terminate_backend and the statistics info as discussed previously. It sounds like

Re: [HACKERS] Review of GetUserId() Usage

2014-12-02 Thread Stephen Frost
Robert, * Robert Haas (robertmh...@gmail.com) wrote: On Sat, Nov 29, 2014 at 3:00 PM, Stephen Frost sfr...@snowman.net wrote: * Stephen Frost (sfr...@snowman.net) wrote: Updated patch attached which also changes the error messages (which hadn't been updated in the prior versions and really

Re: [HACKERS] Review of GetUserId() Usage

2014-12-02 Thread Stephen Frost
* Stephen Frost (sfr...@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

Re: [HACKERS] Review of GetUserId() Usage

2014-12-02 Thread Robert Haas
On Tue, Dec 2, 2014 at 2:50 PM, Stephen Frost sfr...@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

Re: [HACKERS] Review of GetUserId() Usage

2014-11-29 Thread Stephen Frost
All, * Stephen Frost (sfr...@snowman.net) wrote: * Stephen Frost (sfr...@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.

Re: [HACKERS] Review of GetUserId() Usage

2014-10-27 Thread Stephen Frost
* Stephen Frost (sfr...@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 diff --git

Re: [HACKERS] Review of GetUserId() Usage

2014-10-27 Thread Stephen Frost
All, * Peter Eisentraut (pete...@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.

Re: [HACKERS] Review of GetUserId() Usage

2014-10-18 Thread Brightwell, Adam
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 -

Re: [HACKERS] Review of GetUserId() Usage

2014-10-16 Thread Stephen Frost
* Peter Eisentraut (pete...@gmx.net) wrote: On 9/24/14 4:58 PM, Stephen Frost wrote: * Alvaro Herrera (alvhe...@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

Re: [HACKERS] Review of GetUserId() Usage

2014-10-16 Thread Stephen Frost
Robert, On Thursday, October 16, 2014, Robert Haas robertmh...@gmail.com wrote: On Thu, Oct 16, 2014 at 9:49 AM, Stephen Frost sfr...@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

Re: [HACKERS] Review of GetUserId() Usage

2014-10-16 Thread Robert Haas
On Thu, Oct 16, 2014 at 11:28 AM, Stephen Frost sfr...@snowman.net wrote: On Thursday, October 16, 2014, Robert Haas robertmh...@gmail.com wrote: On Thu, Oct 16, 2014 at 9:49 AM, Stephen Frost sfr...@snowman.net wrote: As a side-note, this change is included in the 'role attributes' patch.

Re: [HACKERS] Review of GetUserId() Usage

2014-10-16 Thread Stephen Frost
* Robert Haas (robertmh...@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

Re: [HACKERS] Review of GetUserId() Usage

2014-10-16 Thread Brightwell, Adam
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.

Re: [HACKERS] Review of GetUserId() Usage

2014-09-25 Thread Peter Eisentraut
On 9/24/14 4:58 PM, Stephen Frost wrote: Alvaro, * Alvaro Herrera (alvhe...@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

Re: [HACKERS] Review of GetUserId() Usage

2014-09-25 Thread Stephen Frost
* Peter Eisentraut (pete...@gmx.net) wrote: On 9/24/14 4:58 PM, Stephen Frost wrote: * Alvaro Herrera (alvhe...@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

Re: [HACKERS] Review of GetUserId() Usage

2014-09-24 Thread Stephen Frost
Alvaro, * Alvaro Herrera (alvhe...@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

[HACKERS] Review of GetUserId() Usage

2014-09-23 Thread Stephen Frost
Greetings, While looking through the GetUserId() usage in the backend, I couldn't help but notice a few rather questionable cases that, in my view, should be cleaned up. As a reminder, GetUserId() returns the OID of the user we are generally operating as (eg: whatever the 'role' is, as

Re: [HACKERS] Review of GetUserId() Usage

2014-09-23 Thread Alvaro Herrera
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