Re: superuser() shortcuts - Mailing list pgsql-hackers
| From | Adam Brightwell |
|---|---|
| Subject | Re: superuser() shortcuts |
| Date | |
| Msg-id | CAKRt6CS7y6BXMRhbuiMuO-69SeZUBF7jHt2W5k_zrQd71yA9vQ@mail.gmail.com Whole thread Raw |
| In response to | Re: superuser() shortcuts (Stephen Frost <sfrost@snowman.net>) |
| List | pgsql-hackers |
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..
Thanks,
Adam
--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
Attachment
pgsql-hackers by date: