Re: Repetition of warning message while REVOKE

Hi,

Description:
===========
Repetition of warning message with revoke.

How to reproduce :
==================

> create table tbl(col int);
> create user usr;
> grant select on tbl to usr;
> \c postgres usr;
> REVOKE SELECT on tbl from usr;

Actual output:
================
WARNING: no privileges could be revoked for "tbl"
WARNING: no privileges could be revoked for "tbl"
WARNING: no privileges could be revoked for "tbl"
WARNING: no privileges could be revoked for "tbl"
WARNING: no privileges could be revoked for "tbl"
WARNING: no privileges could be revoked for "tbl"
WARNING: no privileges could be revoked for "tbl"
WARNING: no privileges could be revoked for "tbl"
REVOKE

expected output:
===============
Shouldn't print repetitive warnings.

--
Piyush S Newe
Principal Engineer
EnterpriseDB
office: +91 20 3058 9500
www.enterprisedb.com

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are not
the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

Piyush Newe <piyush(dot)newe(at)enterprisedb(dot)com> writes:
> create table tbl(col int);
> create user usr;
> grant select on tbl to usr;
> \c postgres usr;
> REVOKE SELECT on tbl from usr;
> WARNING: no privileges could be revoked for "tbl"
> WARNING: no privileges could be revoked for "tbl"
> WARNING: no privileges could be revoked for "tbl"
> WARNING: no privileges could be revoked for "tbl"
> WARNING: no privileges could be revoked for "tbl"
> WARNING: no privileges could be revoked for "tbl"
> WARNING: no privileges could be revoked for "tbl"
> WARNING: no privileges could be revoked for "tbl"
> REVOKE

You really should mention what version you're testing, but for the
archives: I confirm this on 8.4.x and HEAD. 8.3 seems to behave sanely.

regards, tom lane

I wrote:
> Piyush Newe <piyush(dot)newe(at)enterprisedb(dot)com> writes:
>> create table tbl(col int);
>> create user usr;
>> grant select on tbl to usr;
>> \c postgres usr;
>> REVOKE SELECT on tbl from usr;
>> WARNING: no privileges could be revoked for "tbl"
>> WARNING: no privileges could be revoked for "tbl"
>> WARNING: no privileges could be revoked for "tbl"
>> WARNING: no privileges could be revoked for "tbl"
>> WARNING: no privileges could be revoked for "tbl"
>> WARNING: no privileges could be revoked for "tbl"
>> WARNING: no privileges could be revoked for "tbl"
>> WARNING: no privileges could be revoked for "tbl"
>> REVOKE

> You really should mention what version you're testing, but for the
> archives: I confirm this on 8.4.x and HEAD. 8.3 seems to behave sanely.

I traced through this and determined that the extra messages are a
consequence of the column-level-privileges patch.
restrict_and_check_grant is invoked both on the whole relation, and
on each column (since we have to get rid of any per-column SELECT
privilege that might have been granted).

I'm not sure offhand about a reasonable way to rearrange the code to
avoid duplicate messages.

regards, tom lane

On Thu, 2010-03-04 at 11:23 -0500, Tom Lane wrote:
> I wrote:
> > Piyush Newe <piyush(dot)newe(at)enterprisedb(dot)com> writes:
> >> create table tbl(col int);
> >> create user usr;
> >> grant select on tbl to usr;
> >> \c postgres usr;
> >> REVOKE SELECT on tbl from usr;
> >> WARNING: no privileges could be revoked for "tbl"
> >> WARNING: no privileges could be revoked for "tbl"
> >> WARNING: no privileges could be revoked for "tbl"
> >> WARNING: no privileges could be revoked for "tbl"
> >> WARNING: no privileges could be revoked for "tbl"
> >> WARNING: no privileges could be revoked for "tbl"
> >> WARNING: no privileges could be revoked for "tbl"
> >> WARNING: no privileges could be revoked for "tbl"
> >> REVOKE
>
> > You really should mention what version you're testing, but for the
> > archives: I confirm this on 8.4.x and HEAD. 8.3 seems to behave sanely.
>
> I traced through this and determined that the extra messages are a
> consequence of the column-level-privileges patch.
> restrict_and_check_grant is invoked both on the whole relation, and
> on each column (since we have to get rid of any per-column SELECT
> privilege that might have been granted).
>
> I'm not sure offhand about a reasonable way to rearrange the code to
> avoid duplicate messages.

Perhaps just add what can't be revoked? meaning:

WARNING: no privileges could be revoked for "tbl" for column "foo"

Then they aren't actually duplicate.

Sincerely,

Joshau D. Drake

>
> regards, tom lane
>

--
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564
Consulting, Training, Support, Custom Development, Engineering
Respect is earned, not gained through arbitrary and repetitive use or Mr. or Sir.

All,

* Joshua D. Drake (jd(at)commandprompt(dot)com) wrote:
> On Thu, 2010-03-04 at 11:23 -0500, Tom Lane wrote:
> > I'm not sure offhand about a reasonable way to rearrange the code to
> > avoid duplicate messages.
>
> Perhaps just add what can't be revoked? meaning:
> WARNING: no privileges could be revoked for "tbl" for column "foo"
> Then they aren't actually duplicate.

Yeah, they really aren't, after all. I don't know how we could
rearrange the code to prevent it- we're checking and trying to change
privileges on each of the columns in the table, after all.

Attached is a patch to add column name to the error message when it's a
column-level failure. I'm not really thrilled with it, due to the
expansion of code and addition of a bunch of conditionals, but at least
this isn't a terribly complicated function..

In the process of trying to build/run regression tests, but having
some build issues w/ HEAD (probably my fault).

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Joshua D. Drake (jd(at)commandprompt(dot)com) wrote:
>> Perhaps just add what can't be revoked? meaning:
>> WARNING: no privileges could be revoked for "tbl" for column "foo"
>> Then they aren't actually duplicate.

> Yeah, they really aren't, after all.

Yeah, I agree JD's solution is probably the simplest reasonable answer.

> Attached is a patch to add column name to the error message when it's a
> column-level failure. I'm not really thrilled with it, due to the
> expansion of code and addition of a bunch of conditionals, but at least
> this isn't a terribly complicated function..

It's a bit brute-force, but so was the original coding. Anybody see
a way to make it cleaner/shorter?

One thought is that the column cases should be phrased more like
no privileges could be revoked for column "foo" of table "bar"
Check the messages associated with DROP cascading for the canonical
phrasing here, but I think that's what it is.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> One thought is that the column cases should be phrased more like
> no privileges could be revoked for column "foo" of table "bar"
> Check the messages associated with DROP cascading for the canonical
> phrasing here, but I think that's what it is.

Looks like 'for column "foo" of relation "bar"' is more typical, so
that's what I did in the attached patch. I also cleaned up a few other
things I noticed in looking through the various messages/comments.

Thanks!

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> One thought is that the column cases should be phrased more like
>> no privileges could be revoked for column "foo" of table "bar"

> Looks like 'for column "foo" of relation "bar"' is more typical, so
> that's what I did in the attached patch. I also cleaned up a few other
> things I noticed in looking through the various messages/comments.

Applied, except I omitted the one comment change because it didn't seem
to me to clarify anything. Sequences are a subclass of relations, so
"table or sequence" makes sense to me while "relation or sequence"
doesn't.

regards, tom lane