Re: Review of GetUserId() Usage

Robert,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Sat, Nov 29, 2014 at 3:00 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Stephen Frost (sfrost(at)snowman(dot)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