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.newe@enterprisedb.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.newe@enterprisedb.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.newe@enterprisedb.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"
REVOKEYou 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@commandprompt.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
Attachments:
fixdups.patchtext/x-diff; charset=us-asciiDownload
Index: src/backend/catalog/aclchk.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/catalog/aclchk.c,v
retrieving revision 1.163
diff -c -r1.163 aclchk.c
*** src/backend/catalog/aclchk.c 26 Feb 2010 02:00:35 -0000 1.163
--- src/backend/catalog/aclchk.c 5 Mar 2010 01:18:42 -0000
***************
*** 304,327 ****
if (is_grant)
{
if (this_privileges == 0)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg("no privileges were granted for \"%s\"", objname)));
! else if (!all_privs && this_privileges != privileges)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg("not all privileges were granted for \"%s\"", objname)));
}
else
{
if (this_privileges == 0)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg("no privileges could be revoked for \"%s\"", objname)));
! else if (!all_privs && this_privileges != privileges)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg("not all privileges could be revoked for \"%s\"", objname)));
}
return this_privileges;
--- 304,365 ----
if (is_grant)
{
if (this_privileges == 0)
! {
! if (objkind == ACL_KIND_COLUMN && colname)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg("no privileges were granted for \"%s\", for column \"%s\"",
! objname, colname)));
! else
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg("no privileges were granted for \"%s\"", objname)));
! }
! else
! {
! if (!all_privs && this_privileges != privileges)
! {
! if (objkind == ACL_KIND_COLUMN && colname)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg("not all privileges were granted for \"%s\", for column \"%s\"",
! objname, colname)));
! else
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg("not all privileges were granted for \"%s\"", objname)));
! }
! }
}
else
{
if (this_privileges == 0)
! {
! if (objkind == ACL_KIND_COLUMN && colname)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg("no privileges could be revoked for \"%s\", for column \"%s\"",
! objname, colname)));
! else
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg("no privileges could be revoked for \"%s\"", objname)));
! }
! else
! {
! if (!all_privs && this_privileges != privileges)
! {
! if (objkind == ACL_KIND_COLUMN && colname)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg("not all privileges could be revoked for \"%s\", for column \"%s\"",
! objname, colname)));
! else
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg("not all privileges could be revoked for \"%s\"", objname)));
! }
! }
}
return this_privileges;
Stephen Frost <sfrost@snowman.net> writes:
* Joshua D. Drake (jd@commandprompt.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@sss.pgh.pa.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
Attachments:
fixdups_20100305.patchtext/x-diff; charset=us-asciiDownload
? src/backend/regex/.regcomp.c.swp
? src/pl/plpgsql/src/pl_scan.c
Index: src/backend/catalog/aclchk.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/catalog/aclchk.c,v
retrieving revision 1.163
diff -c -r1.163 aclchk.c
*** src/backend/catalog/aclchk.c 26 Feb 2010 02:00:35 -0000 1.163
--- src/backend/catalog/aclchk.c 5 Mar 2010 13:16:48 -0000
***************
*** 304,327 ****
if (is_grant)
{
if (this_privileges == 0)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg("no privileges were granted for \"%s\"", objname)));
! else if (!all_privs && this_privileges != privileges)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg("not all privileges were granted for \"%s\"", objname)));
}
else
{
if (this_privileges == 0)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg("no privileges could be revoked for \"%s\"", objname)));
! else if (!all_privs && this_privileges != privileges)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg("not all privileges could be revoked for \"%s\"", objname)));
}
return this_privileges;
--- 304,365 ----
if (is_grant)
{
if (this_privileges == 0)
! {
! if (objkind == ACL_KIND_COLUMN && colname)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg("no privileges were granted for column \"%s\" of relation \"%s\"",
! colname, objname)));
! else
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg("no privileges were granted for \"%s\"", objname)));
! }
! else
! {
! if (!all_privs && this_privileges != privileges)
! {
! if (objkind == ACL_KIND_COLUMN && colname)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg("not all privileges were granted for column \"%s\" of relation \"%s\"",
! colname, objname)));
! else
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
! errmsg("not all privileges were granted for \"%s\"", objname)));
! }
! }
}
else
{
if (this_privileges == 0)
! {
! if (objkind == ACL_KIND_COLUMN && colname)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg("no privileges could be revoked for column \"%s\" of relation \"%s\"",
! colname, objname)));
! else
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg("no privileges could be revoked for \"%s\"", objname)));
! }
! else
! {
! if (!all_privs && this_privileges != privileges)
! {
! if (objkind == ACL_KIND_COLUMN && colname)
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg("not all privileges could be revoked for column \"%s\" of relation \"%s\"",
! colname, objname)));
! else
! ereport(WARNING,
! (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
! errmsg("not all privileges could be revoked for \"%s\"", objname)));
! }
! }
}
return this_privileges;
***************
*** 1657,1664 ****
/*
* The GRANT TABLE syntax can be used for sequences and non-sequences,
* so we have to look at the relkind to determine the supported
! * permissions. The OR of table and sequence permissions were already
! * checked.
*/
if (istmt->objtype == ACL_OBJECT_RELATION)
{
--- 1695,1702 ----
/*
* The GRANT TABLE syntax can be used for sequences and non-sequences,
* so we have to look at the relkind to determine the supported
! * permissions. The OR of relation and sequence permissions were
! * already checked.
*/
if (istmt->objtype == ACL_OBJECT_RELATION)
{
***************
*** 3046,3052 ****
case ACLCHECK_NO_PRIV:
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("permission denied for column %s of relation %s",
colname, objectname)));
break;
case ACLCHECK_NOT_OWNER:
--- 3084,3090 ----
case ACLCHECK_NO_PRIV:
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("permission denied for column \"%s\" of relation \"%s\"",
colname, objectname)));
break;
case ACLCHECK_NOT_OWNER:
Stephen Frost <sfrost@snowman.net> writes:
* Tom Lane (tgl@sss.pgh.pa.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