Re: superuser() shortcuts

All,

> It was brought up for discussion- see
>
> http://www.postgresql.org/message-id/20141015052259.GG28859@tamriel.snowman.net
>
> Specifically:
> ---------------
> One curious item to note is that the
> current if(!superuser()) {} block approach has masked an inconsistency
> in at least the FDW code which required a change to the regression
> tests- namely, we normally force FDW owners to have USAGE rights on
> the FDW, but that was being bypassed when a superuser makes the call.
> I seriously doubt that was intentional. I won't push back if someone
> really wants it to stay that way though.
> ---------------
>
> No one mentioned any concerns with it (and three people replied), so I'm
> inclined to think it's an agreeable change. Adam probably didn't
> mention it with this patch simply because it had already been brought
> up.

Yes, this is correct, I was under the impression that this has already been
addressed. Also, I thought it is a relatively benign change and perhaps
even one for the better. With that said, I'll certainly leave it as is if
that is the consensus.

> > Which makes me wonder whether the other changes are indeed without
> > effect or just not covered by tests.
> >
> > > * has_privilege-cleanup_11-5-2014.patch
> >
> > I don't really see the merit of this patch. A "cleanup" patch that adds
> > more code than it removes isn't really a cleanup. If you want to make
> > the APIs consistent, then let's make a patch for that. If you want to
> > make the error messages consistent, then let's make a patch for that.
>

Fair enough. I think we all agree that fixing the superuser shortcuts are
a relevant and welcome change (at least that is the sense I get). So, how
about for the time being, we table the "consistent API and error messages"
and focus solely on the shortcuts? If that is favorable, then I have
attached a patch to address those changes.

> There is other work going on replacing these role attributes with
> > something more general, so maybe this cleanup should be done as part of
> > that other work.
>

I agree and given the work that has already been done toward that effort I
think that would perhaps be the better approach to addressing them.

Perhaps 'cleanup' is just an overloaded term. The patch is to make the
> APIs and the error messages consistent. I'm amazed at how much
> discussion and work is going into these exceptionally minor changes
> which have been already broken out of the larger and far more
> interesting work (imv anyway). Having two patches, one to simply move
> the checks around and then another to make the error messages in those
> checks consistent, which will naturally end up depending on each other,
> strikes me as overkill, but we can certainly do it.
>

Agreed, but will certainly do whatever is necessary to keep these changes
moving forward. Though, I think the attached patch mitigates any need to
break these changes out further.

Andres raised a concern about the specific error message wording (which
> was intended to just make it more consistent with the rest of our
> permission-check error messages..), are there any other opinions on the
> wording? Would be great to get feedback on that..
>

Agreed, I would certainly be willing to move these changes forward as a
separate effort, but without more specific recommendations beyond what has
already been discussed and proposed I'm at a bit of a loss on where to take
it. I'm all ears on this one and would certainly appreciate any feedback

Thanks,
Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com