and it's not a bunny rabbit, either
In reviewing the work Shigeru Hanada has done on SQL/MED, it's come to
my attention that we have a lot of error messages that use the error
code ERRCODE_WRONG_OBJECT_TYPE and have text like this:
"%s" is not a table
"%s" is not an index
or even better:
"%s" is not a table, view, composite type, or index
which, once we have foreign tables, needs to be changed to read:
"%s" is not a table, view, composite type, index, or foreign table
If we someday add materialized views, it'll need to mention that, too.
In the particular case I'm looking at right now (renameatt_internal),
a more informative error message might be something like
"system-generated attribute names may not be altered", and maybe
that's actually a good way to go in that particular case. But it
seems somewhat painful to make this work for ATSimplePermissions() and
ATSimplePermissionsRelationOrIndex(); in many cases, there's not much
to say beyond the fact that the particular alter table subcommand
isn't supported by the object type to which the user has attempted to
apply it. Still, I'm a bit wondering if there's some more generic way
we can phrase the problem.
Could we get away with something as simple as "requested operation is
not supported for <plural-form-of-object-type>"? In some sense that's
less informative, because it doesn't tell you which object types DO
support the requested operation, but it's not clear that you care
about that. If you are trying to drop a column from a view, the fact
that it would be possible to drop a column from a table is cold
comfort. The advantage of this method is that you need only N
translatable strings (one per relkind), rather than 2^N (one per
subset of the universe of relkinds).
A slightly more granular version of this would be to make the caller
pass in a string indicating what the requested operation actually is,
so that you can say something like "<plural-form-of-object-type> do
not support <requested-operation>" or "<requested-operation> is not
supported by <plural-form-of-object-type>" (e.g. "views do not support
SET NOT NULL"). But that breaks our guideline about assembling
translatable strings from small pieces. Maybe it'd be OK if the piece
is just a fragment of SQL syntax, though - not sure.
Or we can just stick with the way we've been doing it, if I'm the only
one who thinks it's icky.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Sun, Dec 26, 2010 at 10:13:35PM -0500, Robert Haas wrote:
In reviewing the work Shigeru Hanada has done on SQL/MED, it's come to
my attention that we have a lot of error messages that use the error
code ERRCODE_WRONG_OBJECT_TYPE and have text like this:"%s" is not a table
"%s" is not an indexor even better:
"%s" is not a table, view, composite type, or index
which, once we have foreign tables, needs to be changed to read:
"%s" is not a table, view, composite type, index, or foreign table
If we someday add materialized views, it'll need to mention that, too.
In the particular case I'm looking at right now (renameatt_internal),
a more informative error message might be something like
"system-generated attribute names may not be altered", and maybe
that's actually a good way to go in that particular case. But it
seems somewhat painful to make this work for ATSimplePermissions() and
ATSimplePermissionsRelationOrIndex(); in many cases, there's not much
to say beyond the fact that the particular alter table subcommand
isn't supported by the object type to which the user has attempted to
apply it. Still, I'm a bit wondering if there's some more generic way
we can phrase the problem.Could we get away with something as simple as "requested operation is
not supported for <plural-form-of-object-type>"?
+1 for this. It's clear, informative, and relevant to the error at
hand.
In some sense that's
less informative, because it doesn't tell you which object types DO
support the requested operation, but it's not clear that you care
about that. If you are trying to drop a column from a view, the fact
that it would be possible to drop a column from a table is cold
comfort. The advantage of this method is that you need only N
translatable strings (one per relkind), rather than 2^N (one per
subset of the universe of relkinds).A slightly more granular version of this would be to make the caller
pass in a string indicating what the requested operation actually is,
so that you can say something like "<plural-form-of-object-type> do
not support <requested-operation>" or "<requested-operation> is not
supported by <plural-form-of-object-type>" (e.g. "views do not support
SET NOT NULL"). But that breaks our guideline about assembling
translatable strings from small pieces. Maybe it'd be OK if the piece
is just a fragment of SQL syntax, though - not sure.Or we can just stick with the way we've been doing it, if I'm the only
one who thinks it's icky.
You're not the only one.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
On Mon, Dec 27, 2010 at 12:13, Robert Haas <robertmhaas@gmail.com> wrote:
Could we get away with something as simple as "requested operation is
not supported for <plural-form-of-object-type>"?
+1. If so, will we have a function to get object names something like
GetPluralFormOfObjectType(Relation rel or char relkind) => char * ?
But that breaks our guideline about assembling
translatable strings from small pieces. Maybe it'd be OK if the piece
is just a fragment of SQL syntax, though - not sure.
Agreed. <requested-operation> should be a SQL syntax,
so we won't have to translate the part.
--
Itagaki Takahiro
On Sun, Dec 26, 2010 at 10:44 PM, Itagaki Takahiro
<itagaki.takahiro@gmail.com> wrote:
On Mon, Dec 27, 2010 at 12:13, Robert Haas <robertmhaas@gmail.com> wrote:
Could we get away with something as simple as "requested operation is
not supported for <plural-form-of-object-type>"?+1. If so, will we have a function to get object names something like
GetPluralFormOfObjectType(Relation rel or char relkind) => char * ?
In the interest of keeping things simple for translators, I was
thinking we'd just write out a string for each object type:
"requested operation is not supported for tables"
"requested operation is not supported for views"
"requested operation is not supported for indexes"
or if we go with the some-assembly required version, perhaps:
"tables do not support %s"
"views do not support %s"
"indexes do not support %s"
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Dec 26, 2010, at 7:55 PM, Robert Haas wrote:
"tables do not support %s"
"views do not support %s"
"indexes do not support %s"
The more detail we can give, the better, of course. Nothing's more frustrating than having a command with an error like, "Object does not support requested operation." Thanks, computer program: "Swerved off road, hit tree" is about as useful.
--
-- Christophe Pettus
xof@thebuild.com
Robert Haas <robertmhaas@gmail.com> writes:
or if we go with the some-assembly required version, perhaps:
"tables do not support %s"
"views do not support %s"
"indexes do not support %s"
+1 for that way. Although personally I'd reverse the phrasing:
/* translator: %s is the name of a SQL command */
%s does not support tables
regards, tom lane
On Mon, Dec 27, 2010 at 10:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
or if we go with the some-assembly required version, perhaps:
"tables do not support %s"
"views do not support %s"
"indexes do not support %s"+1 for that way. Although personally I'd reverse the phrasing:
/* translator: %s is the name of a SQL command */
%s does not support tables
To me, it seems as though it's the object which doesn't support the
operation, rather than the other way around. Reversing it makes most
sense to me in cases where it's an implementation restriction, such as
"DROP COLUMN does not support views". But at least to me, the
phrasing "SET WITHOUT CLUSTER does not support views" suggests that
SET WITHOUT CLUSTER is somehow defective, which is not really the
message I think we want to convey there. But maybe we need some other
votes.
I took a crack at implementing this and the results were mixed - see
attached patch. It works pretty well for the most part, but there are
a couple of warts. For ALTER TABLE commands like DROP COLUMN and SET
WITHOUT CLUSTER the results look pretty good, and I find the new error
messages a definite improvement over the old ones. It's not quite so
good for setting reloptions or attoptions. You get something like:
ERROR: views do not support SET (...)
ERROR: views do not support ALTER COLUMN SET (...)
...which might actually be OK, if not fantastically wonderful. One
might think of mitigating this problem by writing "ALTER TABLE SET
(...)" rather than just "SET (...)", but that's not easily done
because there are several equivalent forms (for example, a view can be
modified with either ALTER VIEW or ALTER TABLE, for historical - or
perhaps hysterical - reasons). An even bigger problem is this:
rhaas=# alter view v add primary key (a);
ERROR: views do not support ADD INDEX
The problem is that alter table actions AT_AddIndex and
AT_AddConstraint don't tie neatly back to a particular piece of
syntax. The message as written isn't incomprehensible (especially if
you're reading it in English) but it definitely leaves something to be
desired.
Ideas?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
not-a-whatever.patchtext/x-patch; charset=US-ASCII; name=not-a-whatever.patchDownload
diff --git a/src/backend/commands/comment.c b/src/backend/commands/comment.c
index b578818..9701233 100644
--- a/src/backend/commands/comment.c
+++ b/src/backend/commands/comment.c
@@ -22,6 +22,7 @@
#include "catalog/pg_shdescription.h"
#include "commands/comment.h"
#include "commands/dbcommands.h"
+#include "commands/tablecmds.h"
#include "libpq/be-fsstubs.h"
#include "miscadmin.h"
#include "parser/parse_func.h"
@@ -583,10 +584,7 @@ CheckAttributeComment(Relation relation)
if (relation->rd_rel->relkind != RELKIND_RELATION &&
relation->rd_rel->relkind != RELKIND_VIEW &&
relation->rd_rel->relkind != RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, or composite type",
- RelationGetRelationName(relation))));
+ ErrorWrongRelationType(relation, "COMMENT ON COLUMN");
}
/*
diff --git a/src/backend/commands/seclabel.c b/src/backend/commands/seclabel.c
index 762bbae..00b64bf 100644
--- a/src/backend/commands/seclabel.c
+++ b/src/backend/commands/seclabel.c
@@ -366,10 +366,7 @@ CheckAttributeSecLabel(Relation relation)
if (relation->rd_rel->relkind != RELKIND_RELATION &&
relation->rd_rel->relkind != RELKIND_VIEW &&
relation->rd_rel->relkind != RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, or composite type",
- RelationGetRelationName(relation))));
+ ErrorWrongRelationType(relation, "SECURITY LABEL ON COLUMN");
}
void
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6729d83..ee410c4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -222,6 +222,44 @@ static const struct dropmsgstrings dropmsgstringarray[] = {
{'\0', 0, NULL, NULL, NULL, NULL}
};
+/*
+ * Error-reporting support for wrong-object type errors
+ */
+struct wrongtypestrings
+{
+ char kind;
+ const char *wrongtype_message;
+};
+
+static const struct wrongtypestrings wrongtypestringarray[] = {
+ {RELKIND_RELATION,
+ /* translator: %s is an SQL command */
+ gettext_noop("tables do not support %s")},
+ {RELKIND_SEQUENCE,
+ /* translator: %s is an SQL command */
+ gettext_noop("sequences do not support %s")},
+ {RELKIND_VIEW,
+ /* translator: %s is an SQL command */
+ gettext_noop("views do not support %s")},
+ {RELKIND_INDEX,
+ /* translator: %s is an SQL command */
+ gettext_noop("indexes do not support %s")},
+ {RELKIND_COMPOSITE_TYPE,
+ /* translator: %s is an SQL command */
+ gettext_noop("composite types do not support %s")},
+ {RELKIND_TOASTVALUE,
+ /* translator: %s is an SQL command */
+ gettext_noop("TOAST tables do not support %s")},
+ {'\0', NULL}
+};
+
+/* Convenience strings for ATSimplePermissions */
+const char rk_relation[2] = { RELKIND_RELATION, '\0' };
+const char rk_view[2] = { RELKIND_VIEW, '\0' };
+const char rk_relation_view[3] = { RELKIND_RELATION, RELKIND_VIEW, '\0' };
+const char rk_relation_index[3] = { RELKIND_RELATION, RELKIND_INDEX, '\0' };
+const char rk_relation_type[3] =
+ { RELKIND_RELATION, RELKIND_COMPOSITE_TYPE, '\0' };
static void truncate_check_rel(Relation rel);
static List *MergeAttributes(List *schema, List *supers, char relpersistence,
@@ -264,8 +302,8 @@ static void ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
static void ATRewriteTables(List **wqueue, LOCKMODE lockmode);
static void ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode);
static AlteredTableInfo *ATGetQueueEntry(List **wqueue, Relation rel);
-static void ATSimplePermissions(Relation rel, bool allowView, bool allowType);
-static void ATSimplePermissionsRelationOrIndex(Relation rel);
+static void ATSimplePermissions(Relation rel, const char *relkinds,
+ const char *command);
static void ATSimpleRecursion(List **wqueue, Relation rel,
AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode);
static void ATOneLevelRecursion(List **wqueue, Relation rel,
@@ -1096,10 +1134,7 @@ truncate_check_rel(Relation rel)
/* Only allow truncate on regular tables */
if (rel->rd_rel->relkind != RELKIND_RELATION)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table",
- RelationGetRelationName(rel))));
+ ErrorWrongRelationType(rel, "TRUNCATE");
/* Permissions checks */
aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
@@ -1994,8 +2029,7 @@ renameatt_internal(Oid myrelid,
relkind != RELKIND_INDEX)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, composite type or index",
- RelationGetRelationName(targetrelation))));
+ errmsg("system-generated column names may not be altered")));
/*
* permissions checking. only the owner of a class can change its schema.
@@ -2684,14 +2718,14 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
switch (cmd->subtype)
{
case AT_AddColumn: /* ADD COLUMN */
- ATSimplePermissions(rel, false, true);
+ ATSimplePermissions(rel, rk_relation_type, "ADD COLUMN");
/* Performs own recursion */
ATPrepAddColumn(wqueue, rel, recurse, recursing, cmd, lockmode);
pass = AT_PASS_ADD_COL;
break;
case AT_AddColumnToView: /* add column via CREATE OR REPLACE
* VIEW */
- ATSimplePermissions(rel, true, false);
+ ATSimplePermissions(rel, rk_view, "ADD COLUMN");
/* Performs own recursion */
ATPrepAddColumn(wqueue, rel, recurse, recursing, cmd, lockmode);
pass = AT_PASS_ADD_COL;
@@ -2704,19 +2738,20 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
* substitutes default values into INSERTs before it expands
* rules.
*/
- ATSimplePermissions(rel, true, false);
+ ATSimplePermissions(rel, rk_relation_view,
+ "ALTER COLUMN DEFAULT");
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
/* No command-specific prep needed */
pass = cmd->def ? AT_PASS_ADD_CONSTR : AT_PASS_DROP;
break;
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation, "DROP NOT NULL");
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
/* No command-specific prep needed */
pass = AT_PASS_DROP;
break;
case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation, "SET NOT NULL");
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
/* No command-specific prep needed */
pass = AT_PASS_ADD_CONSTR;
@@ -2728,31 +2763,37 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_MISC;
break;
case AT_SetOptions: /* ALTER COLUMN SET ( options ) */
+ /* This command never recurses */
+ ATSimplePermissions(rel, rk_relation_index,
+ "ALTER COLUMN SET (...)");
+ pass = AT_PASS_MISC;
+ break;
case AT_ResetOptions: /* ALTER COLUMN RESET ( options ) */
- ATSimplePermissionsRelationOrIndex(rel);
/* This command never recurses */
+ ATSimplePermissions(rel, rk_relation_index,
+ "ALTER COLUMN RESET (...)");
pass = AT_PASS_MISC;
break;
case AT_SetStorage: /* ALTER COLUMN SET STORAGE */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation, "ALTER COLUMN SET STORAGE");
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_DropColumn: /* DROP COLUMN */
- ATSimplePermissions(rel, false, true);
+ ATSimplePermissions(rel, rk_relation_type, "DROP COLUMN");
ATPrepDropColumn(wqueue, rel, recurse, recursing, cmd, lockmode);
/* Recursion occurs during execution phase */
pass = AT_PASS_DROP;
break;
case AT_AddIndex: /* ADD INDEX */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation, "ADD INDEX");
/* This command never recurses */
/* No command-specific prep needed */
pass = AT_PASS_ADD_INDEX;
break;
case AT_AddConstraint: /* ADD CONSTRAINT */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation, "ADD CONSTRAINT");
/* Recursion occurs during execution phase */
/* No command-specific prep needed except saving recurse flag */
if (recurse)
@@ -2760,7 +2801,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_ADD_CONSTR;
break;
case AT_DropConstraint: /* DROP CONSTRAINT */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation, "DROP CONSTRAINT");
/* Recursion occurs during execution phase */
/* No command-specific prep needed except saving recurse flag */
if (recurse)
@@ -2768,7 +2809,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_DROP;
break;
case AT_AlterColumnType: /* ALTER COLUMN TYPE */
- ATSimplePermissions(rel, false, true);
+ ATSimplePermissions(rel, rk_relation_type, "ALTER COLUMN TYPE");
/* Performs own recursion */
ATPrepAlterColumnType(wqueue, tab, rel, recurse, recursing, cmd, lockmode);
pass = AT_PASS_ALTER_TYPE;
@@ -2779,21 +2820,26 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_MISC;
break;
case AT_ClusterOn: /* CLUSTER ON */
+ ATSimplePermissions(rel, rk_relation, "CLUSTER ON");
+ /* These commands never recurse */
+ /* No command-specific prep needed */
+ pass = AT_PASS_MISC;
+ break;
case AT_DropCluster: /* SET WITHOUT CLUSTER */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation, "SET WITHOUT CLUSTER");
/* These commands never recurse */
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_AddOids: /* SET WITH OIDS */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation, "SET WITH OIDS");
/* Performs own recursion */
if (!rel->rd_rel->relhasoids || recursing)
ATPrepAddOids(wqueue, rel, recurse, cmd, lockmode);
pass = AT_PASS_ADD_COL;
break;
case AT_DropOids: /* SET WITHOUT OIDS */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation, "SET WITHOUT OIDS");
/* Performs own recursion */
if (rel->rd_rel->relhasoids)
{
@@ -2807,38 +2853,82 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_DROP;
break;
case AT_SetTableSpace: /* SET TABLESPACE */
- ATSimplePermissionsRelationOrIndex(rel);
+ ATSimplePermissions(rel, rk_relation_index, "SET TABLESPACE");
/* This command never recurses */
ATPrepSetTableSpace(tab, rel, cmd->name, lockmode);
pass = AT_PASS_MISC; /* doesn't actually matter */
break;
case AT_SetRelOptions: /* SET (...) */
+ ATSimplePermissions(rel, rk_relation_index, "SET (...)");
+ /* This command never recurses */
+ /* No command-specific prep needed */
+ pass = AT_PASS_MISC;
case AT_ResetRelOptions: /* RESET (...) */
- ATSimplePermissionsRelationOrIndex(rel);
+ ATSimplePermissions(rel, rk_relation_index, "RESET (...)");
/* This command never recurses */
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_AddInherit: /* INHERIT */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation, "INHERIT");
/* This command never recurses */
ATPrepAddInherit(rel);
pass = AT_PASS_MISC;
break;
- case AT_EnableTrig: /* ENABLE TRIGGER variants */
- case AT_EnableAlwaysTrig:
- case AT_EnableReplicaTrig:
+ case AT_EnableTrig: /* ENABLE TRIGGER variants */
case AT_EnableTrigAll:
case AT_EnableTrigUser:
+ ATSimplePermissions(rel, rk_relation, "ENABLE TRIGGER");
+ /* These commands never recurse */
+ /* No command-specific prep needed */
+ pass = AT_PASS_MISC;
+ break;
+ case AT_EnableAlwaysTrig:
+ ATSimplePermissions(rel, rk_relation, "ENABLE ALWAYS TRIGGER");
+ /* These commands never recurse */
+ /* No command-specific prep needed */
+ pass = AT_PASS_MISC;
+ break;
+ case AT_EnableReplicaTrig:
+ ATSimplePermissions(rel, rk_relation, "ENABLE REPLICA TRIGGER");
+ /* These commands never recurse */
+ /* No command-specific prep needed */
+ pass = AT_PASS_MISC;
+ break;
case AT_DisableTrig: /* DISABLE TRIGGER variants */
case AT_DisableTrigAll:
case AT_DisableTrigUser:
+ ATSimplePermissions(rel, rk_relation, "DISABLE TRIGGER");
+ /* These commands never recurse */
+ /* No command-specific prep needed */
+ pass = AT_PASS_MISC;
+ break;
case AT_EnableRule: /* ENABLE/DISABLE RULE variants */
+ ATSimplePermissions(rel, rk_relation, "ENABLE RULE");
+ /* These commands never recurse */
+ /* No command-specific prep needed */
+ pass = AT_PASS_MISC;
+ break;
case AT_EnableAlwaysRule:
+ ATSimplePermissions(rel, rk_relation, "ENABLE ALWAYS RULE");
+ /* These commands never recurse */
+ /* No command-specific prep needed */
+ pass = AT_PASS_MISC;
+ break;
case AT_EnableReplicaRule:
+ ATSimplePermissions(rel, rk_relation, "ENABLE REPLICA RULE");
+ /* These commands never recurse */
+ /* No command-specific prep needed */
+ pass = AT_PASS_MISC;
+ break;
case AT_DisableRule:
+ ATSimplePermissions(rel, rk_relation, "DISABLE RULE");
+ /* These commands never recurse */
+ /* No command-specific prep needed */
+ pass = AT_PASS_MISC;
+ break;
case AT_DropInherit: /* NO INHERIT */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation, "NO INHERIT");
/* These commands never recurse */
/* No command-specific prep needed */
pass = AT_PASS_MISC;
@@ -3559,66 +3649,15 @@ ATGetQueueEntry(List **wqueue, Relation rel)
/*
* ATSimplePermissions
*
- * - Ensure that it is a relation (or possibly a view)
+ * - Check the relkind against the provided list
* - Ensure this user is the owner
* - Ensure that it is not a system table
*/
static void
-ATSimplePermissions(Relation rel, bool allowView, bool allowType)
+ATSimplePermissions(Relation rel, const char *relkinds, const char *command)
{
- if (rel->rd_rel->relkind != RELKIND_RELATION)
- {
- if (allowView)
- {
- if (rel->rd_rel->relkind != RELKIND_VIEW)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table or view",
- RelationGetRelationName(rel))));
- }
- else if (allowType)
- {
- if (rel->rd_rel->relkind != RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table or composite type",
- RelationGetRelationName(rel))));
- }
- else
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table",
- RelationGetRelationName(rel))));
- }
-
- /* Permissions checks */
- if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
- aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
- RelationGetRelationName(rel));
-
- if (!allowSystemTableMods && IsSystemRelation(rel))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- RelationGetRelationName(rel))));
-}
-
-/*
- * ATSimplePermissionsRelationOrIndex
- *
- * - Ensure that it is a relation or an index
- * - Ensure this user is the owner
- * - Ensure that it is not a system table
- */
-static void
-ATSimplePermissionsRelationOrIndex(Relation rel)
-{
- if (rel->rd_rel->relkind != RELKIND_RELATION &&
- rel->rd_rel->relkind != RELKIND_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table or index",
- RelationGetRelationName(rel))));
+ if (!strchr(relkinds, rel->rd_rel->relkind))
+ ErrorWrongRelationType(rel, command);
/* Permissions checks */
if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
@@ -4449,10 +4488,7 @@ ATPrepSetStatistics(Relation rel, const char *colName, Node *newValue, LOCKMODE
*/
if (rel->rd_rel->relkind != RELKIND_RELATION &&
rel->rd_rel->relkind != RELKIND_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table or index",
- RelationGetRelationName(rel))));
+ ErrorWrongRelationType(rel, "ALTER COLUMN .. SET STATISTICS");
/* Permissions checks */
if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
@@ -4692,7 +4728,7 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
/* At top level, permission check was done in ATPrepCmd, else do it */
if (recursing)
- ATSimplePermissions(rel, false, true);
+ ATSimplePermissions(rel, rk_relation_type, "DROP COLUMN");
/*
* get the number of the attribute
@@ -4996,7 +5032,7 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
/* At top level, permission check was done in ATPrepCmd, else do it */
if (recursing)
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation, "ADD CONSTRAINT");
/*
* Call AddRelationNewConstraints to do the work, making sure it works on
@@ -5940,7 +5976,7 @@ ATExecDropConstraint(Relation rel, const char *constrName,
/* At top level, permission check was done in ATPrepCmd, else do it */
if (recursing)
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation, "DROP CONSTRAINT");
conrel = heap_open(ConstraintRelationId, RowExclusiveLock);
@@ -6881,10 +6917,7 @@ ATExecChangeOwner(Oid relationOid, Oid newOwnerId, bool recursing, LOCKMODE lock
break;
/* FALL THRU */
default:
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, or sequence",
- NameStr(tuple_class->relname))));
+ ErrorWrongRelationType(target_rel, "OWNER TO");
}
/*
@@ -7188,10 +7221,8 @@ ATExecSetRelOptions(Relation rel, List *defList, bool isReset, LOCKMODE lockmode
(void) index_reloptions(rel->rd_am->amoptions, newOptions, true);
break;
default:
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, index, or TOAST table",
- RelationGetRelationName(rel))));
+ ErrorWrongRelationType(rel, isReset ?
+ "RESET (...)" : "SET (...)");
break;
}
@@ -7543,7 +7574,7 @@ ATExecAddInherit(Relation child_rel, RangeVar *parent, LOCKMODE lockmode)
* Must be owner of both parent and child -- child was checked by
* ATSimplePermissions call in ATPrepCmd
*/
- ATSimplePermissions(parent_rel, false, false);
+ ATSimplePermissions(parent_rel, rk_relation, "INHERIT");
/* Permanent rels cannot inherit from temporary ones */
if (RelationUsesTempNamespace(parent_rel)
@@ -8186,10 +8217,7 @@ AlterTableNamespace(RangeVar *relation, const char *newschema,
case RELKIND_TOASTVALUE:
/* FALL THRU */
default:
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, or sequence",
- RelationGetRelationName(rel))));
+ ErrorWrongRelationType(rel, "SET SCHEMA");
}
/* get schema OID and check its permissions */
@@ -8376,6 +8404,22 @@ AlterSeqNamespaces(Relation classRel, Relation rel,
relation_close(depRel, AccessShareLock);
}
+/*
+ * Emit the right error message for a SQL command issue on a relation of
+ * the wrong type.
+ */
+void
+ErrorWrongRelationType(Relation rel, const char *command)
+{
+ const struct wrongtypestrings *entry;
+
+ for (entry = wrongtypestringarray; entry->kind != '\0'; entry++)
+ if (entry->kind == rel->rd_rel->relkind)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg(entry->wrongtype_message, command)));
+ elog(ERROR, "unexpected relkind: %c", rel->rd_rel->relkind);
+}
/*
* This code supports
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index ad932d3..d802128 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -60,6 +60,8 @@ extern AttrNumber *varattnos_map(TupleDesc olddesc, TupleDesc newdesc);
extern AttrNumber *varattnos_map_schema(TupleDesc old, List *schema);
extern void change_varattnos_of_a_node(Node *node, const AttrNumber *newattno);
+extern void ErrorWrongRelationType(Relation rel, const char *command);
+
extern void register_on_commit_action(Oid relid, OnCommitAction action);
extern void remove_on_commit_action(Oid relid);
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index e415730..b7917ea 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -599,9 +599,9 @@ ERROR: cannot alter system column "oid"
-- try creating a view and altering that, should fail
create view myview as select * from atacc1;
alter table myview alter column test drop not null;
-ERROR: "myview" is not a table
+ERROR: views do not support DROP NOT NULL
alter table myview alter column test set not null;
-ERROR: "myview" is not a table
+ERROR: views do not support SET NOT NULL
drop view myview;
drop table atacc1;
-- test inheritance
@@ -854,7 +854,7 @@ select * from myview;
(0 rows)
alter table myview drop d;
-ERROR: "myview" is not a table or composite type
+ERROR: views do not support DROP COLUMN
drop view myview;
-- test some commands to make sure they fail on the dropped column
analyze atacc1(a);
On Mon, Dec 27, 2010 at 2:06 PM, Robert Haas <robertmhaas@gmail.com> wrote:
The problem is that alter table actions AT_AddIndex and
AT_AddConstraint don't tie neatly back to a particular piece of
syntax. The message as written isn't incomprehensible (especially if
you're reading it in English) but it definitely leaves something to be
desired.Ideas?
Here's a somewhat more complete patch implementing this concept, plus
adding additional messages for non-support of constraints, rules, and
triggers. More could be done in this vein, but this picks a decent
fraction of the low-hanging fruit.
I've had to change some of the heap_open(rv) calls to
relation_open(rv) to avoid having the former throw the wrong error
message before the latter kicks in. I think there might be stylistic
objections to that, but I'm not sure what else to propose. I'm
actually pretty suspicious that many of the heap_open(rv) calls I
*didn't* change are either already a little iffy or likely to become
so once the SQL/MED stuff for foreign tables goes in. They make it
easy to forget that we've got a whole pile of relkinds and you
actually need to really think about which ones you can handle.
For example, on unpatched head:
rhaas=# create view v as select 1 as a;
CREATE VIEW
rhaas=# cluster v;
ERROR: there is no previously clustered index for table "v"
The error message is demonstrably correct in the sense that, first,
there isn't any table v, only a view v, so surely table v has no
clustered index - or anything else; and second, even if we construe
table "v" to mean view "v", it is certainly right to say it has no
clustered index because it does not - and can not - have any indexes
at all. But as undeniably true as that error message is, it's a bad
error message. With the patch:
rhaas=# cluster v;
ERROR: views do not support CLUSTER
That's more like it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
not-a-whatever-v2.patchtext/x-patch; charset=US-ASCII; name=not-a-whatever-v2.patchDownload
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 249067f..1555b61 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -113,7 +113,9 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
Relation rel;
/* Find and lock the table */
- rel = heap_openrv(stmt->relation, AccessExclusiveLock);
+ rel = relation_openrv(stmt->relation, AccessExclusiveLock);
+ if (rel->rd_rel->relkind != RELKIND_RELATION)
+ ErrorWrongRelkind(rel, WRONG_RELKIND_FOR_COMMAND, "CLUSTER");
tableOid = RelationGetRelid(rel);
diff --git a/src/backend/commands/comment.c b/src/backend/commands/comment.c
index b578818..66df9f8 100644
--- a/src/backend/commands/comment.c
+++ b/src/backend/commands/comment.c
@@ -22,6 +22,7 @@
#include "catalog/pg_shdescription.h"
#include "commands/comment.h"
#include "commands/dbcommands.h"
+#include "commands/tablecmds.h"
#include "libpq/be-fsstubs.h"
#include "miscadmin.h"
#include "parser/parse_func.h"
@@ -583,10 +584,8 @@ CheckAttributeComment(Relation relation)
if (relation->rd_rel->relkind != RELKIND_RELATION &&
relation->rd_rel->relkind != RELKIND_VIEW &&
relation->rd_rel->relkind != RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, or composite type",
- RelationGetRelationName(relation))));
+ ErrorWrongRelkind(relation, WRONG_RELKIND_FOR_COMMAND,
+ "COMMENT ON COLUMN");
}
/*
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 7b8bee8..488cc80 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -27,6 +27,7 @@
#include "catalog/pg_type.h"
#include "commands/copy.h"
#include "commands/defrem.h"
+#include "commands/tablecmds.h"
#include "commands/trigger.h"
#include "executor/executor.h"
#include "libpq/libpq.h"
@@ -998,8 +999,19 @@ DoCopy(const CopyStmt *stmt, const char *queryString)
cstate->queryDesc = NULL;
/* Open and lock the relation, using the appropriate lock type. */
- cstate->rel = heap_openrv(stmt->relation,
+ cstate->rel = relation_openrv(stmt->relation,
(is_from ? RowExclusiveLock : AccessShareLock));
+ if (cstate->rel->rd_rel->relkind != RELKIND_RELATION)
+ {
+ if (!is_from && cstate->rel->rd_rel->relkind == RELKIND_VIEW)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("views do not support COPY TO"),
+ errhint("Try the COPY (SELECT ...) TO variant.")));
+ else
+ ErrorWrongRelkind(cstate->rel, WRONG_RELKIND_FOR_COMMAND,
+ is_from ? "COPY FROM" : "COPY TO");
+ }
tupDesc = RelationGetDescr(cstate->rel);
@@ -1225,29 +1237,6 @@ DoCopyTo(CopyState cstate)
{
bool pipe = (cstate->filename == NULL);
- if (cstate->rel)
- {
- if (cstate->rel->rd_rel->relkind != RELKIND_RELATION)
- {
- if (cstate->rel->rd_rel->relkind == RELKIND_VIEW)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot copy from view \"%s\"",
- RelationGetRelationName(cstate->rel)),
- errhint("Try the COPY (SELECT ...) TO variant.")));
- else if (cstate->rel->rd_rel->relkind == RELKIND_SEQUENCE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot copy from sequence \"%s\"",
- RelationGetRelationName(cstate->rel))));
- else
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot copy from non-table relation \"%s\"",
- RelationGetRelationName(cstate->rel))));
- }
- }
-
if (pipe)
{
if (whereToSendOutput == DestRemote)
@@ -1701,25 +1690,6 @@ CopyFrom(CopyState cstate)
Assert(cstate->rel);
- if (cstate->rel->rd_rel->relkind != RELKIND_RELATION)
- {
- if (cstate->rel->rd_rel->relkind == RELKIND_VIEW)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot copy to view \"%s\"",
- RelationGetRelationName(cstate->rel))));
- else if (cstate->rel->rd_rel->relkind == RELKIND_SEQUENCE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot copy to sequence \"%s\"",
- RelationGetRelationName(cstate->rel))));
- else
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot copy to non-table relation \"%s\"",
- RelationGetRelationName(cstate->rel))));
- }
-
/*----------
* Check to see if we can avoid writing WAL
*
diff --git a/src/backend/commands/seclabel.c b/src/backend/commands/seclabel.c
index 762bbae..5c7bf29 100644
--- a/src/backend/commands/seclabel.c
+++ b/src/backend/commands/seclabel.c
@@ -366,10 +366,7 @@ CheckAttributeSecLabel(Relation relation)
if (relation->rd_rel->relkind != RELKIND_RELATION &&
relation->rd_rel->relkind != RELKIND_VIEW &&
relation->rd_rel->relkind != RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, or composite type",
- RelationGetRelationName(relation))));
+ ErrorWrongRelkind(relation, "SECURITY LABEL ON COLUMN");
}
void
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6729d83..80017e8 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -222,6 +222,65 @@ static const struct dropmsgstrings dropmsgstringarray[] = {
{'\0', 0, NULL, NULL, NULL, NULL}
};
+/*
+ * Error-reporting support for wrong-object type errors
+ */
+struct wrongtypestrings
+{
+ char kind;
+ const char *command_message;
+ const char *constraint_message;
+ const char *trigger_message;
+ const char *rule_message;
+};
+
+static const struct wrongtypestrings wrongtypestringarray[] = {
+ {RELKIND_RELATION,
+ /* translator: %s is an SQL command */
+ gettext_noop("tables do not support %s"),
+ NULL, /* constraints are supported, so no message */
+ NULL, /* rules are supported, so no message */
+ NULL}, /* triggers are supported, so no message */
+ {RELKIND_SEQUENCE,
+ /* translator: %s is an SQL command */
+ gettext_noop("sequences do not support %s"),
+ gettext_noop("sequences do not support constraints"),
+ gettext_noop("sequences do not support rules"),
+ gettext_noop("sequences do not support triggers")},
+ {RELKIND_VIEW,
+ /* translator: %s is an SQL command */
+ gettext_noop("views do not support %s"),
+ gettext_noop("views do not support constraints"),
+ NULL, /* rules are supported, so no message */
+ NULL}, /* triggers are supported, so no message */
+ {RELKIND_INDEX,
+ /* translator: %s is an SQL command */
+ gettext_noop("indexes do not support %s"),
+ gettext_noop("indexes do not support constraints"),
+ gettext_noop("indexes do not support rules"),
+ gettext_noop("indexes do not support triggers")},
+ {RELKIND_COMPOSITE_TYPE,
+ /* translator: %s is an SQL command */
+ gettext_noop("composite types do not support %s"),
+ gettext_noop("composite types do not support constraints"),
+ gettext_noop("composite types do not support rules"),
+ gettext_noop("composite types do not support triggers")},
+ {RELKIND_TOASTVALUE,
+ /* translator: %s is an SQL command */
+ gettext_noop("TOAST tables do not support %s"),
+ gettext_noop("TOAST tables do not support constraints"),
+ gettext_noop("TOAST tables do not support rules"),
+ gettext_noop("TOAST tables do not support triggers")},
+ {'\0', NULL, NULL, NULL}
+};
+
+/* Convenience strings for ATSimplePermissions */
+const char rk_relation[2] = { RELKIND_RELATION, '\0' };
+const char rk_view[2] = { RELKIND_VIEW, '\0' };
+const char rk_relation_view[3] = { RELKIND_RELATION, RELKIND_VIEW, '\0' };
+const char rk_relation_index[3] = { RELKIND_RELATION, RELKIND_INDEX, '\0' };
+const char rk_relation_type[3] =
+ { RELKIND_RELATION, RELKIND_COMPOSITE_TYPE, '\0' };
static void truncate_check_rel(Relation rel);
static List *MergeAttributes(List *schema, List *supers, char relpersistence,
@@ -264,8 +323,8 @@ static void ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
static void ATRewriteTables(List **wqueue, LOCKMODE lockmode);
static void ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode);
static AlteredTableInfo *ATGetQueueEntry(List **wqueue, Relation rel);
-static void ATSimplePermissions(Relation rel, bool allowView, bool allowType);
-static void ATSimplePermissionsRelationOrIndex(Relation rel);
+static void ATSimplePermissions(Relation rel, const char *relkinds,
+ WrongRelkindDetail detail, const char *command);
static void ATSimpleRecursion(List **wqueue, Relation rel,
AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode);
static void ATOneLevelRecursion(List **wqueue, Relation rel,
@@ -841,7 +900,7 @@ ExecuteTruncate(TruncateStmt *stmt)
bool recurse = interpretInhOption(rv->inhOpt);
Oid myrelid;
- rel = heap_openrv(rv, AccessExclusiveLock);
+ rel = relation_openrv(rv, AccessExclusiveLock);
myrelid = RelationGetRelid(rel);
/* don't throw error for "TRUNCATE foo, foo" */
if (list_member_oid(relids, myrelid))
@@ -868,7 +927,7 @@ ExecuteTruncate(TruncateStmt *stmt)
continue;
/* find_all_inheritors already got lock */
- rel = heap_open(childrelid, NoLock);
+ rel = relation_open(childrelid, NoLock);
truncate_check_rel(rel);
rels = lappend(rels, rel);
relids = lappend_oid(relids, childrelid);
@@ -899,7 +958,7 @@ ExecuteTruncate(TruncateStmt *stmt)
Oid relid = lfirst_oid(cell);
Relation rel;
- rel = heap_open(relid, AccessExclusiveLock);
+ rel = relation_open(relid, AccessExclusiveLock);
ereport(NOTICE,
(errmsg("truncate cascades to table \"%s\"",
RelationGetRelationName(rel))));
@@ -1096,10 +1155,7 @@ truncate_check_rel(Relation rel)
/* Only allow truncate on regular tables */
if (rel->rd_rel->relkind != RELKIND_RELATION)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table",
- RelationGetRelationName(rel))));
+ ErrorWrongRelkind(rel, WRONG_RELKIND_FOR_COMMAND, "TRUNCATE");
/* Permissions checks */
aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
@@ -1994,8 +2050,7 @@ renameatt_internal(Oid myrelid,
relkind != RELKIND_INDEX)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, composite type or index",
- RelationGetRelationName(targetrelation))));
+ errmsg("system-generated column names may not be altered")));
/*
* permissions checking. only the owner of a class can change its schema.
@@ -2684,14 +2739,17 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
switch (cmd->subtype)
{
case AT_AddColumn: /* ADD COLUMN */
- ATSimplePermissions(rel, false, true);
+ ATSimplePermissions(rel, rk_relation_type,
+ WRONG_RELKIND_FOR_COMMAND, "ADD COLUMN");
/* Performs own recursion */
ATPrepAddColumn(wqueue, rel, recurse, recursing, cmd, lockmode);
pass = AT_PASS_ADD_COL;
break;
case AT_AddColumnToView: /* add column via CREATE OR REPLACE
* VIEW */
- ATSimplePermissions(rel, true, false);
+ ATSimplePermissions(rel, rk_view,
+ WRONG_RELKIND_FOR_COMMAND,
+ "ADD COLUMN");
/* Performs own recursion */
ATPrepAddColumn(wqueue, rel, recurse, recursing, cmd, lockmode);
pass = AT_PASS_ADD_COL;
@@ -2704,19 +2762,25 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
* substitutes default values into INSERTs before it expands
* rules.
*/
- ATSimplePermissions(rel, true, false);
+ ATSimplePermissions(rel, rk_relation_view,
+ WRONG_RELKIND_FOR_COMMAND,
+ "ALTER COLUMN DEFAULT");
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
/* No command-specific prep needed */
pass = cmd->def ? AT_PASS_ADD_CONSTR : AT_PASS_DROP;
break;
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation,
+ WRONG_RELKIND_FOR_COMMAND,
+ "DROP NOT NULL");
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
/* No command-specific prep needed */
pass = AT_PASS_DROP;
break;
case AT_SetNotNull: /* ALTER COLUMN SET NOT NULL */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation,
+ WRONG_RELKIND_FOR_COMMAND,
+ "SET NOT NULL");
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
/* No command-specific prep needed */
pass = AT_PASS_ADD_CONSTR;
@@ -2728,31 +2792,47 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_MISC;
break;
case AT_SetOptions: /* ALTER COLUMN SET ( options ) */
+ /* This command never recurses */
+ ATSimplePermissions(rel, rk_relation_index,
+ WRONG_RELKIND_FOR_COMMAND,
+ "ALTER COLUMN SET (...)");
+ pass = AT_PASS_MISC;
+ break;
case AT_ResetOptions: /* ALTER COLUMN RESET ( options ) */
- ATSimplePermissionsRelationOrIndex(rel);
/* This command never recurses */
+ ATSimplePermissions(rel, rk_relation_index,
+ WRONG_RELKIND_FOR_COMMAND,
+ "ALTER COLUMN RESET (...)");
pass = AT_PASS_MISC;
break;
case AT_SetStorage: /* ALTER COLUMN SET STORAGE */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation,
+ WRONG_RELKIND_FOR_COMMAND,
+ "ALTER COLUMN SET STORAGE");
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_DropColumn: /* DROP COLUMN */
- ATSimplePermissions(rel, false, true);
+ ATSimplePermissions(rel, rk_relation_type,
+ WRONG_RELKIND_FOR_COMMAND,
+ "DROP COLUMN");
ATPrepDropColumn(wqueue, rel, recurse, recursing, cmd, lockmode);
/* Recursion occurs during execution phase */
pass = AT_PASS_DROP;
break;
case AT_AddIndex: /* ADD INDEX */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation,
+ WRONG_RELKIND_FOR_CONSTRAINTS,
+ NULL);
/* This command never recurses */
/* No command-specific prep needed */
pass = AT_PASS_ADD_INDEX;
break;
case AT_AddConstraint: /* ADD CONSTRAINT */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation,
+ WRONG_RELKIND_FOR_CONSTRAINTS,
+ NULL);
/* Recursion occurs during execution phase */
/* No command-specific prep needed except saving recurse flag */
if (recurse)
@@ -2760,7 +2840,9 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_ADD_CONSTR;
break;
case AT_DropConstraint: /* DROP CONSTRAINT */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation,
+ WRONG_RELKIND_FOR_CONSTRAINTS,
+ NULL);
/* Recursion occurs during execution phase */
/* No command-specific prep needed except saving recurse flag */
if (recurse)
@@ -2768,7 +2850,9 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_DROP;
break;
case AT_AlterColumnType: /* ALTER COLUMN TYPE */
- ATSimplePermissions(rel, false, true);
+ ATSimplePermissions(rel, rk_relation_type,
+ WRONG_RELKIND_FOR_COMMAND,
+ "ALTER COLUMN TYPE");
/* Performs own recursion */
ATPrepAlterColumnType(wqueue, tab, rel, recurse, recursing, cmd, lockmode);
pass = AT_PASS_ALTER_TYPE;
@@ -2779,21 +2863,34 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_MISC;
break;
case AT_ClusterOn: /* CLUSTER ON */
+ ATSimplePermissions(rel, rk_relation,
+ WRONG_RELKIND_FOR_COMMAND,
+ "CLUSTER ON");
+ /* These commands never recurse */
+ /* No command-specific prep needed */
+ pass = AT_PASS_MISC;
+ break;
case AT_DropCluster: /* SET WITHOUT CLUSTER */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation,
+ WRONG_RELKIND_FOR_COMMAND,
+ "SET WITHOUT CLUSTER");
/* These commands never recurse */
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_AddOids: /* SET WITH OIDS */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation,
+ WRONG_RELKIND_FOR_COMMAND,
+ "SET WITH OIDS");
/* Performs own recursion */
if (!rel->rd_rel->relhasoids || recursing)
ATPrepAddOids(wqueue, rel, recurse, cmd, lockmode);
pass = AT_PASS_ADD_COL;
break;
case AT_DropOids: /* SET WITHOUT OIDS */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation,
+ WRONG_RELKIND_FOR_COMMAND,
+ "SET WITHOUT OIDS");
/* Performs own recursion */
if (rel->rd_rel->relhasoids)
{
@@ -2807,20 +2904,32 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_DROP;
break;
case AT_SetTableSpace: /* SET TABLESPACE */
- ATSimplePermissionsRelationOrIndex(rel);
+ ATSimplePermissions(rel, rk_relation_index,
+ WRONG_RELKIND_FOR_COMMAND,
+ "SET TABLESPACE");
/* This command never recurses */
ATPrepSetTableSpace(tab, rel, cmd->name, lockmode);
pass = AT_PASS_MISC; /* doesn't actually matter */
break;
case AT_SetRelOptions: /* SET (...) */
+ ATSimplePermissions(rel, rk_relation_index,
+ WRONG_RELKIND_FOR_COMMAND,
+ "SET (...)");
+ /* This command never recurses */
+ /* No command-specific prep needed */
+ pass = AT_PASS_MISC;
case AT_ResetRelOptions: /* RESET (...) */
- ATSimplePermissionsRelationOrIndex(rel);
+ ATSimplePermissions(rel, rk_relation_index,
+ WRONG_RELKIND_FOR_COMMAND,
+ "RESET (...)");
/* This command never recurses */
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_AddInherit: /* INHERIT */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation,
+ WRONG_RELKIND_FOR_COMMAND,
+ "INHERIT");
/* This command never recurses */
ATPrepAddInherit(rel);
pass = AT_PASS_MISC;
@@ -2833,12 +2942,28 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
case AT_DisableTrig: /* DISABLE TRIGGER variants */
case AT_DisableTrigAll:
case AT_DisableTrigUser:
+ ATSimplePermissions(rel, rk_relation,
+ WRONG_RELKIND_FOR_TRIGGERS,
+ NULL);
+ /* These commands never recurse */
+ /* No command-specific prep needed */
+ pass = AT_PASS_MISC;
+ break;
case AT_EnableRule: /* ENABLE/DISABLE RULE variants */
case AT_EnableAlwaysRule:
case AT_EnableReplicaRule:
case AT_DisableRule:
+ ATSimplePermissions(rel, rk_relation,
+ WRONG_RELKIND_FOR_RULES,
+ NULL);
+ /* These commands never recurse */
+ /* No command-specific prep needed */
+ pass = AT_PASS_MISC;
+ break;
case AT_DropInherit: /* NO INHERIT */
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation,
+ WRONG_RELKIND_FOR_COMMAND,
+ "NO INHERIT");
/* These commands never recurse */
/* No command-specific prep needed */
pass = AT_PASS_MISC;
@@ -3559,66 +3684,16 @@ ATGetQueueEntry(List **wqueue, Relation rel)
/*
* ATSimplePermissions
*
- * - Ensure that it is a relation (or possibly a view)
- * - Ensure this user is the owner
- * - Ensure that it is not a system table
- */
-static void
-ATSimplePermissions(Relation rel, bool allowView, bool allowType)
-{
- if (rel->rd_rel->relkind != RELKIND_RELATION)
- {
- if (allowView)
- {
- if (rel->rd_rel->relkind != RELKIND_VIEW)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table or view",
- RelationGetRelationName(rel))));
- }
- else if (allowType)
- {
- if (rel->rd_rel->relkind != RELKIND_COMPOSITE_TYPE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table or composite type",
- RelationGetRelationName(rel))));
- }
- else
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table",
- RelationGetRelationName(rel))));
- }
-
- /* Permissions checks */
- if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
- aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
- RelationGetRelationName(rel));
-
- if (!allowSystemTableMods && IsSystemRelation(rel))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- RelationGetRelationName(rel))));
-}
-
-/*
- * ATSimplePermissionsRelationOrIndex
- *
- * - Ensure that it is a relation or an index
+ * - Check the relkind against the provided list
* - Ensure this user is the owner
* - Ensure that it is not a system table
*/
static void
-ATSimplePermissionsRelationOrIndex(Relation rel)
+ATSimplePermissions(Relation rel, const char *relkinds,
+ WrongRelkindDetail detail, const char *command)
{
- if (rel->rd_rel->relkind != RELKIND_RELATION &&
- rel->rd_rel->relkind != RELKIND_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table or index",
- RelationGetRelationName(rel))));
+ if (!strchr(relkinds, rel->rd_rel->relkind))
+ ErrorWrongRelkind(rel, detail, command);
/* Permissions checks */
if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
@@ -4449,10 +4524,8 @@ ATPrepSetStatistics(Relation rel, const char *colName, Node *newValue, LOCKMODE
*/
if (rel->rd_rel->relkind != RELKIND_RELATION &&
rel->rd_rel->relkind != RELKIND_INDEX)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table or index",
- RelationGetRelationName(rel))));
+ ErrorWrongRelkind(rel, WRONG_RELKIND_FOR_COMMAND,
+ "ALTER COLUMN .. SET STATISTICS");
/* Permissions checks */
if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
@@ -4692,7 +4765,9 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
/* At top level, permission check was done in ATPrepCmd, else do it */
if (recursing)
- ATSimplePermissions(rel, false, true);
+ ATSimplePermissions(rel, rk_relation_type,
+ WRONG_RELKIND_FOR_COMMAND,
+ "DROP COLUMN");
/*
* get the number of the attribute
@@ -4996,7 +5071,9 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
/* At top level, permission check was done in ATPrepCmd, else do it */
if (recursing)
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation,
+ WRONG_RELKIND_FOR_CONSTRAINTS,
+ NULL);
/*
* Call AddRelationNewConstraints to do the work, making sure it works on
@@ -5940,7 +6017,9 @@ ATExecDropConstraint(Relation rel, const char *constrName,
/* At top level, permission check was done in ATPrepCmd, else do it */
if (recursing)
- ATSimplePermissions(rel, false, false);
+ ATSimplePermissions(rel, rk_relation,
+ WRONG_RELKIND_FOR_CONSTRAINTS,
+ NULL);
conrel = heap_open(ConstraintRelationId, RowExclusiveLock);
@@ -6881,10 +6960,8 @@ ATExecChangeOwner(Oid relationOid, Oid newOwnerId, bool recursing, LOCKMODE lock
break;
/* FALL THRU */
default:
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, or sequence",
- NameStr(tuple_class->relname))));
+ ErrorWrongRelkind(target_rel, WRONG_RELKIND_FOR_COMMAND,
+ "OWNER TO");
}
/*
@@ -7188,10 +7265,8 @@ ATExecSetRelOptions(Relation rel, List *defList, bool isReset, LOCKMODE lockmode
(void) index_reloptions(rel->rd_am->amoptions, newOptions, true);
break;
default:
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, index, or TOAST table",
- RelationGetRelationName(rel))));
+ ErrorWrongRelkind(rel, WRONG_RELKIND_FOR_COMMAND, isReset ?
+ "RESET (...)" : "SET (...)");
break;
}
@@ -7543,7 +7618,9 @@ ATExecAddInherit(Relation child_rel, RangeVar *parent, LOCKMODE lockmode)
* Must be owner of both parent and child -- child was checked by
* ATSimplePermissions call in ATPrepCmd
*/
- ATSimplePermissions(parent_rel, false, false);
+ ATSimplePermissions(parent_rel, rk_relation,
+ WRONG_RELKIND_FOR_COMMAND,
+ "INHERIT");
/* Permanent rels cannot inherit from temporary ones */
if (RelationUsesTempNamespace(parent_rel)
@@ -8186,10 +8263,8 @@ AlterTableNamespace(RangeVar *relation, const char *newschema,
case RELKIND_TOASTVALUE:
/* FALL THRU */
default:
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, or sequence",
- RelationGetRelationName(rel))));
+ ErrorWrongRelkind(rel, WRONG_RELKIND_FOR_COMMAND,
+ "SET SCHEMA");
}
/* get schema OID and check its permissions */
@@ -8376,6 +8451,50 @@ AlterSeqNamespaces(Relation classRel, Relation rel,
relation_close(depRel, AccessShareLock);
}
+/*
+ * Emit the right error message for a SQL command issue on a relation of
+ * the wrong type.
+ */
+void
+ErrorWrongRelkind(Relation rel, WrongRelkindDetail detail,
+ const char *command)
+{
+ const struct wrongtypestrings *entry;
+
+ for (entry = wrongtypestringarray; entry->kind != '\0'; entry++)
+ if (entry->kind == rel->rd_rel->relkind)
+ break;
+ if (entry->kind == '\0')
+ elog(ERROR, "unexpected relkind: %c", rel->rd_rel->relkind);
+ switch (detail)
+ {
+ case WRONG_RELKIND_FOR_COMMAND:
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg(entry->command_message, command)));
+ break;
+ case WRONG_RELKIND_FOR_CONSTRAINTS:
+ Assert(entry->constraint_message != NULL);
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("%s", _(entry->constraint_message))));
+ break;
+ case WRONG_RELKIND_FOR_TRIGGERS:
+ Assert(entry->trigger_message != NULL);
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("%s", _(entry->trigger_message))));
+ break;
+ case WRONG_RELKIND_FOR_RULES:
+ Assert(entry->rule_message != NULL);
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("%s", _(entry->rule_message))));
+ break;
+ default:
+ elog(ERROR, "unexpected WrongRelkindDetail");
+ }
+}
/*
* This code supports
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 8195392..3c2d352 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -27,6 +27,7 @@
#include "catalog/pg_type.h"
#include "commands/dbcommands.h"
#include "commands/defrem.h"
+#include "commands/tablecmds.h"
#include "commands/trigger.h"
#include "executor/executor.h"
#include "executor/instrument.h"
@@ -149,7 +150,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
* need to take an AccessExclusiveLock to add one of those, just as we do
* with ON SELECT rules.
*/
- rel = heap_openrv(stmt->relation, ShareRowExclusiveLock);
+ rel = relation_openrv(stmt->relation, ShareRowExclusiveLock);
/*
* Triggers must be on tables or views, and there are additional
@@ -187,10 +188,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
errdetail("Views cannot have TRUNCATE triggers.")));
}
else
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table or view",
- RelationGetRelationName(rel))));
+ ErrorWrongRelkind(rel, WRONG_RELKIND_FOR_TRIGGERS, NULL);
if (!allowSystemTableMods && IsSystemRelation(rel))
ereport(ERROR,
@@ -1089,10 +1087,7 @@ RemoveTriggerById(Oid trigOid)
if (rel->rd_rel->relkind != RELKIND_RELATION &&
rel->rd_rel->relkind != RELKIND_VIEW)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table or view",
- RelationGetRelationName(rel))));
+ ErrorWrongRelkind(rel, WRONG_RELKIND_FOR_TRIGGERS, NULL);
if (!allowSystemTableMods && IsSystemRelation(rel))
ereport(ERROR,
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index aa7c144..90bf17e 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -1406,7 +1406,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
int count;
Assert(IsA(inh, RangeVar));
- rel = heap_openrv(inh, AccessShareLock);
+ rel = relation_openrv(inh, AccessShareLock);
if (rel->rd_rel->relkind != RELKIND_RELATION)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index 4354897..d81bce6 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -22,6 +22,7 @@
#include "catalog/objectaccess.h"
#include "catalog/pg_rewrite.h"
#include "catalog/storage.h"
+#include "commands/tablecmds.h"
#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
#include "parser/parse_utilcmd.h"
@@ -255,10 +256,7 @@ DefineQueryRewrite(char *rulename,
*/
if (event_relation->rd_rel->relkind != RELKIND_RELATION &&
event_relation->rd_rel->relkind != RELKIND_VIEW)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table or view",
- RelationGetRelationName(event_relation))));
+ ErrorWrongRelkind(event_relation, WRONG_RELKIND_FOR_RULES, NULL);
if (!allowSystemTableMods && IsSystemRelation(event_relation))
ereport(ERROR,
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index ad932d3..9a16488 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -60,6 +60,16 @@ extern AttrNumber *varattnos_map(TupleDesc olddesc, TupleDesc newdesc);
extern AttrNumber *varattnos_map_schema(TupleDesc old, List *schema);
extern void change_varattnos_of_a_node(Node *node, const AttrNumber *newattno);
+typedef enum
+{
+ WRONG_RELKIND_FOR_COMMAND,
+ WRONG_RELKIND_FOR_CONSTRAINTS,
+ WRONG_RELKIND_FOR_TRIGGERS,
+ WRONG_RELKIND_FOR_RULES
+} WrongRelkindDetail;
+extern void ErrorWrongRelkind(Relation rel, WrongRelkindDetail detail,
+ const char *command);
+
extern void register_on_commit_action(Oid relid, OnCommitAction action);
extern void remove_on_commit_action(Oid relid);
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index e415730..b7917ea 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -599,9 +599,9 @@ ERROR: cannot alter system column "oid"
-- try creating a view and altering that, should fail
create view myview as select * from atacc1;
alter table myview alter column test drop not null;
-ERROR: "myview" is not a table
+ERROR: views do not support DROP NOT NULL
alter table myview alter column test set not null;
-ERROR: "myview" is not a table
+ERROR: views do not support SET NOT NULL
drop view myview;
drop table atacc1;
-- test inheritance
@@ -854,7 +854,7 @@ select * from myview;
(0 rows)
alter table myview drop d;
-ERROR: "myview" is not a table or composite type
+ERROR: views do not support DROP COLUMN
drop view myview;
-- test some commands to make sure they fail on the dropped column
analyze atacc1(a);
diff --git a/src/test/regress/expected/copyselect.out b/src/test/regress/expected/copyselect.out
index cbc140c..d7a04a0 100644
--- a/src/test/regress/expected/copyselect.out
+++ b/src/test/regress/expected/copyselect.out
@@ -30,7 +30,7 @@ copy test1 to stdout;
-- This should fail
--
copy v_test1 to stdout;
-ERROR: cannot copy from view "v_test1"
+ERROR: views do not support COPY TO
HINT: Try the COPY (SELECT ...) TO variant.
--
-- Test COPY (select) TO
@@ -109,9 +109,9 @@ t
-- This should fail
--
\copy v_test1 to stdout
-ERROR: cannot copy from view "v_test1"
+ERROR: views do not support COPY TO
HINT: Try the COPY (SELECT ...) TO variant.
-\copy: ERROR: cannot copy from view "v_test1"
+\copy: ERROR: views do not support COPY TO
HINT: Try the COPY (SELECT ...) TO variant.
--
-- Test \copy (select ...)
On 29.12.2010 06:54, Robert Haas wrote:
With the patch:
rhaas=# cluster v;
ERROR: views do not support CLUSTER
"do not support" sounds like a missing feature, rather than a
nonsensical command. How about something like "CLUSTER cannot be used on
views"
The patch changes a bunch of heap_openrv() calls to relation_openrv().
Perhaps it would be better make the error message something like "\"%s\"
is not a table", and keep the callers unchanged. It's not particularly
useful to repeat the command in the error message, the user should know
what command he issued. Even if it's buried deep in a PL/pgSQL function
or something, it should be clear from the context lines.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Wed, Dec 29, 2010 at 4:09 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
On 29.12.2010 06:54, Robert Haas wrote:
With the patch:
rhaas=# cluster v;
ERROR: views do not support CLUSTER"do not support" sounds like a missing feature, rather than a nonsensical
command. How about something like "CLUSTER cannot be used on views"
I'm fine with flipping the ordering around. I think I like it
marginally better this way, but you and Tom both seem to prefer the
opposite ordering, ergo so be it (barring a sudden influx of contrary
votes).
The patch changes a bunch of heap_openrv() calls to relation_openrv().
Perhaps it would be better make the error message something like "\"%s\" is
not a table", and keep the callers unchanged. It's not particularly useful
to repeat the command in the error message, the user should know what
command he issued. Even if it's buried deep in a PL/pgSQL function or
something, it should be clear from the context lines.
Did you read the whole thread?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 29.12.2010 13:17, Robert Haas wrote:
Did you read the whole thread?
Ah, sorry:
I've had to change some of the heap_open(rv) calls to
relation_open(rv) to avoid having the former throw the wrong error
message before the latter kicks in. I think there might be stylistic
objections to that, but I'm not sure what else to propose. I'm
actually pretty suspicious that many of the heap_open(rv) calls I
*didn't* change are either already a little iffy or likely to become
so once the SQL/MED stuff for foreign tables goes in. They make it
easy to forget that we've got a whole pile of relkinds and you
actually need to really think about which ones you can handle.
Hmm, I believe the idea of heap_open is to check that the relation is
backed by a heap that you can read with heap_beginscan+heap_next. At the
moment that includes normal tables, sequences and toast tables. Foreign
tables would not fall into that category.
Yeah, you're right that most of the callers of heap_open actually want
to a tighter check than that.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
Hmm, I believe the idea of heap_open is to check that the relation is
backed by a heap that you can read with heap_beginscan+heap_next. At the
moment that includes normal tables, sequences and toast tables. Foreign
tables would not fall into that category.
I don't believe that that definition is documented anyplace; if we
decide that's what we want it to mean, some code comments would be in
order.
Yeah, you're right that most of the callers of heap_open actually want
to a tighter check than that.
I think probably most of the physical calls of heap_open are actually
associated with system catalog accesses, and the fact that the code says
heap_open not relation_open has got more to do with copy&paste than any
real thought about what we're specifying.
regards, tom lane
On Dec 29, 2010, at 12:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
Hmm, I believe the idea of heap_open is to check that the relation is
backed by a heap that you can read with heap_beginscan+heap_next. At the
moment that includes normal tables, sequences and toast tables. Foreign
tables would not fall into that category.I don't believe that that definition is documented anyplace; if we
decide that's what we want it to mean, some code comments would be in
order.
The existing comments mention that callers must check that the return value is not a view, if they care. So if there is currently a single coherent definition for what heap_open is supposed to do, it's clearly NOT the one Heikki proposes. My guess is that reality is closer to your theory of "what got cut-and-pasted".
...Robert
Robert Haas <robertmhaas@gmail.com> writes:
The existing comments mention that callers must check that the return
value is not a view, if they care. So if there is currently a single
coherent definition for what heap_open is supposed to do, it's clearly
NOT the one Heikki proposes. My guess is that reality is closer to
your theory of "what got cut-and-pasted".
Well, reality is that in the beginning there was heap_open and
index_open, and nothing else. And there weren't views, so basically
those two functions covered all the interesting types of relations.
We got to the current state of affairs by a series of whatever were the
least invasive code changes at the time; nobody's ever taken a step
back and tried to define what "heap_open" ought to allow from the
standpoint of first principles.
In practice I think it would make sense if heap_open accepts all
relation types on which you can potentially do either a heapscan or
indexscan (offhand those should be the same set of relkinds, I think;
so this is the same in effect as Heikki's proposal, but phrased
differently). So it would have to start rejecting views, and we'd need
to go looking for the consequences of that.
regards, tom lane
Excerpts from Tom Lane's message of mié dic 29 16:29:45 -0300 2010:
In practice I think it would make sense if heap_open accepts all
relation types on which you can potentially do either a heapscan or
indexscan (offhand those should be the same set of relkinds, I think;
so this is the same in effect as Heikki's proposal, but phrased
differently). So it would have to start rejecting views, and we'd need
to go looking for the consequences of that.
This seems a very good idea, but I think we shouldn't let it sink the
current patch.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Tom Lane's message of mié dic 29 16:29:45 -0300 2010:
In practice I think it would make sense if heap_open accepts all
relation types on which you can potentially do either a heapscan or
indexscan (offhand those should be the same set of relkinds, I think;
so this is the same in effect as Heikki's proposal, but phrased
differently). So it would have to start rejecting views, and we'd need
to go looking for the consequences of that.
This seems a very good idea, but I think we shouldn't let it sink the
current patch.
No, but possibly regularizing what heap_open is defined to do would make
Robert's patch simpler.
regards, tom lane
On Wed, Dec 29, 2010 at 3:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Tom Lane's message of mié dic 29 16:29:45 -0300 2010:
In practice I think it would make sense if heap_open accepts all
relation types on which you can potentially do either a heapscan or
indexscan (offhand those should be the same set of relkinds, I think;
so this is the same in effect as Heikki's proposal, but phrased
differently). So it would have to start rejecting views, and we'd need
to go looking for the consequences of that.This seems a very good idea, but I think we shouldn't let it sink the
current patch.No, but possibly regularizing what heap_open is defined to do would make
Robert's patch simpler.
I think that any meaning we assign to heap_open is going to be 95%
arbitrary and of little practical help. There are at least six
different sets of object classes which to which a particular commands
or alter table subcommands can be legally applied: (1) tables only,
(2) views only, (3) tables and views, (4) tables and indexes, (5)
tables and composite types, (6) tables, views, and composite types.
Adding foreign tables promises to add several more combinations
immediately and likely more down the line; if we add materialized
views, that'll add a bunch more. There's not really any single
definition that's going to be a silver bullet. I think the best thing
to do might be to get RID of heap_open(rv) and always use
relation_openrv plus an appropriate relkind test.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Dec 29, 2010 at 4:09 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
On 29.12.2010 06:54, Robert Haas wrote:
With the patch:
rhaas=# cluster v;
ERROR: views do not support CLUSTER"do not support" sounds like a missing feature, rather than a nonsensical
command. How about something like "CLUSTER cannot be used on views"
In the latest version of this patch, I created four translatable
strings per object type:
<blats> do not support %s (where %s is an SQL command)
<blats> do not support constraints
<blats> do not support rules
<blats> do not support triggers
It's reasonable enough to write "CLUSTER cannot be used on views", but
does "constraints cannot be used on views" seems more awkward to me.
Or do we think that's OK?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Dec 29, 2010 at 04:53:47PM -0500, Robert Haas wrote:
On Wed, Dec 29, 2010 at 4:09 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:On 29.12.2010 06:54, Robert Haas wrote:
�With the patch:
rhaas=# cluster v;
ERROR: �views do not support CLUSTER"do not support" sounds like a missing feature, rather than a nonsensical
command. How about something like "CLUSTER cannot be used on views"In the latest version of this patch, I created four translatable
strings per object type:<blats> do not support %s (where %s is an SQL command)
<blats> do not support constraints
<blats> do not support rules
<blats> do not support triggersIt's reasonable enough to write "CLUSTER cannot be used on views", but
does "constraints cannot be used on views" seems more awkward to me.
Or do we think that's OK?
That particular one looks good insofar as it describes reality. With
predicate locks, etc., it may become untrue later, though :)
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
On Wed, Dec 29, 2010 at 5:14 PM, David Fetter <david@fetter.org> wrote:
On Wed, Dec 29, 2010 at 04:53:47PM -0500, Robert Haas wrote:
On Wed, Dec 29, 2010 at 4:09 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:On 29.12.2010 06:54, Robert Haas wrote:
With the patch:
rhaas=# cluster v;
ERROR: views do not support CLUSTER"do not support" sounds like a missing feature, rather than a nonsensical
command. How about something like "CLUSTER cannot be used on views"In the latest version of this patch, I created four translatable
strings per object type:<blats> do not support %s (where %s is an SQL command)
<blats> do not support constraints
<blats> do not support rules
<blats> do not support triggersIt's reasonable enough to write "CLUSTER cannot be used on views", but
does "constraints cannot be used on views" seems more awkward to me.
Or do we think that's OK?That particular one looks good insofar as it describes reality. With
predicate locks, etc., it may become untrue later, though :)
After further thought, I think it makes sense to change this around a
bit and create a family of functions that can be invoked like this:
void check_relation_for_FEATURE_support(Relation rel);
...where FEATURE is constraint, trigger, rule, index, etc. The
function will be defined to throw an error if the relation isn't of a
type that can support the named feature. The error message will be of
the form:
constraints can only be used on tables
triggers can be used only on tables and views
etc.
This avoids the need to define a separate error message for each
unsupported relkind, and I think it's just as informative as, e.g.,
"constraints cannot be used on <whatever object type you tried to
invoke it on>". We can adopt the same language for commands, e.g.:
"CLUSTER can only be used on tables".
Comments?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of jue dic 30 12:47:42 -0300 2010:
After further thought, I think it makes sense to change this around a
bit and create a family of functions that can be invoked like this:void check_relation_for_FEATURE_support(Relation rel);
...where FEATURE is constraint, trigger, rule, index, etc. The
function will be defined to throw an error if the relation isn't of a
type that can support the named feature. The error message will be of
the form:constraints can only be used on tables
triggers can be used only on tables and views
etc.
So this will create a combinatorial explosion of strings to translate?
I liked the other idea because the number of translatable strings was
kept within reasonable bounds.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Thu, Dec 30, 2010 at 11:00 AM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
Excerpts from Robert Haas's message of jue dic 30 12:47:42 -0300 2010:
After further thought, I think it makes sense to change this around a
bit and create a family of functions that can be invoked like this:void check_relation_for_FEATURE_support(Relation rel);
...where FEATURE is constraint, trigger, rule, index, etc. The
function will be defined to throw an error if the relation isn't of a
type that can support the named feature. The error message will be of
the form:constraints can only be used on tables
triggers can be used only on tables and views
etc.So this will create a combinatorial explosion of strings to translate?
I liked the other idea because the number of translatable strings was
kept within reasonable bounds.
No, quite the opposite. With the other approach, you needed:
constraints cannot be used on views
constraints cannot be used on composite types
constraints cannot be used on TOAST tables
constraints cannot be used on indexes
constraints cannot be used on foreign tables
With this, you just need:
constraints can only be used on tables
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
After further thought, I think it makes sense to change this around a
bit and create a family of functions that can be invoked like this:
void check_relation_for_FEATURE_support(Relation rel);
That seems like a reasonable idea, but ...
... The error message will be of the form:
constraints can only be used on tables
triggers can be used only on tables and views
etc.
This avoids the need to define a separate error message for each
unsupported relkind, and I think it's just as informative as, e.g.,
"constraints cannot be used on <whatever object type you tried to
invoke it on>". We can adopt the same language for commands, e.g.:
"CLUSTER can only be used on tables".
ISTM there are four things we might potentially want to state in the
error message: the feature/operation you tried to apply, the name of the
object you tried to apply it to, the type of that object, and the set of
object types that the feature/operation will actually work for. Our
current wording ("foo is not a table or view") covers the second and
fourth of these, though the fourth is stated rather awkwardly. Your
proposal above covers the first and fourth. I'm not happy about leaving
out the object name, because there are going to be cases where people
get this type of error out of a long sequence or nest of operations and
it's not clear what it's talking about. It'd probably be okay to leave
out the actual object type as long as you include its name, though.
One possibility is to break it down like this:
ERROR: foo is a sequence
DETAIL: Triggers can only be used on tables and views.
This could still be emitted by a function such as you suggest, and
indeed that would be the most practical way from both a consistency
and code-size standpoint.
regards, tom lane
Excerpts from Tom Lane's message of jue dic 30 13:49:20 -0300 2010:
One possibility is to break it down like this:
ERROR: foo is a sequence
DETAIL: Triggers can only be used on tables and views.This could still be emitted by a function such as you suggest, and
indeed that would be the most practical way from both a consistency
and code-size standpoint.
This seems good to me. There will only be as many messages as relkinds
we have, plus as many as "features" there are.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Thu, Dec 30, 2010 at 11:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
One possibility is to break it down like this:
ERROR: foo is a sequence
DETAIL: Triggers can only be used on tables and views.This could still be emitted by a function such as you suggest, and
indeed that would be the most practical way from both a consistency
and code-size standpoint.
Great idea. I should have thought of that.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Dec 30, 2010 at 12:32 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Dec 30, 2010 at 11:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
One possibility is to break it down like this:
ERROR: foo is a sequence
DETAIL: Triggers can only be used on tables and views.This could still be emitted by a function such as you suggest, and
indeed that would be the most practical way from both a consistency
and code-size standpoint.Great idea. I should have thought of that.
On further reflection, this can still turn into a laundry list in certain cases.
DETAIL: You can only comment on columns of tables, views, and composite types.
seems less helpful than:
DETAIL: Comments on relations with system-generated column names are
not supported.
I think that for rules, triggers, constraints, and anything that only
works on a single relkind, we can't do much better than to list the
specific object types. But where there's some sort of guiding
principle involved I think we'd do well to articulate it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On further reflection, this can still turn into a laundry list in certain cases.
DETAIL: You can only comment on columns of tables, views, and composite types.
seems less helpful than:
DETAIL: Comments on relations with system-generated column names are
not supported.
I think that for rules, triggers, constraints, and anything that only
works on a single relkind, we can't do much better than to list the
specific object types. But where there's some sort of guiding
principle involved I think we'd do well to articulate it.
I'm unconvinced, because the "guiding principle" is likely to be an
implementation detail that won't actually mean much to users. Your
example above is a case in point --- I do *not* think the average
user will see that as an improvement.
regards, tom lane
On Thu, Dec 30, 2010 at 8:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On further reflection, this can still turn into a laundry list in certain cases.
DETAIL: You can only comment on columns of tables, views, and composite types.
seems less helpful than:
DETAIL: Comments on relations with system-generated column names are
not supported.I think that for rules, triggers, constraints, and anything that only
works on a single relkind, we can't do much better than to list the
specific object types. But where there's some sort of guiding
principle involved I think we'd do well to articulate it.I'm unconvinced, because the "guiding principle" is likely to be an
implementation detail that won't actually mean much to users. Your
example above is a case in point --- I do *not* think the average
user will see that as an improvement.
I think this thread has worked itself around to where it's entirely
pointless. My original complaint was about error messages like this:
"%s" is not a table, view, composite type, or index
which, once we have foreign tables, needs to be changed to read:
"%s" is not a table, view, composite type, index, or foreign table
I think that message is the epitome of worthless, and several other
people agreed. After various proposals of greater and lesser merit,
we've somehow worked around to the suggestion that this should be
reworded to:
ERROR: "%s" is a sequence
DETAIL: Only attributes of tables, views, composite types, indexes, or
foreign tables can be renamed.
While that may be a marginal improvement in clarity, it does
absolutely nothing to address my original complaint, which is that
adding a relkind forces trivial revisions of messages all over the
system, some of which are already excessively long-winded. This
message also does nothing to help the user understand WHY we don't
allow renaming the attributes of his sequence or TOAST table, whereas
the proposed revision does.
The absolute worst offenders are messages of the form:
<blah> is not supported on X, Y, Z, or T.
which now have to be revised to read:
<blah> is not supported on X,Y, Z, T, or W.
This problem could be avoided by writing:
<blah> is supported on A and B
Or:
<blah> is supported only for relation types which quack
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
I think this thread has worked itself around to where it's entirely
pointless.
I understand your frustration, but it's not clear to me that there *is*
any simple solution to this problem. Fundamentally, adding new relkinds
to the system is always going to require running around and looking at a
lot of code to see what's affected; and that goes for the error messages
too. I put no stock at all in the idea that writing a "guiding
principle" in the error messages will avoid anything, because as often
as not, adding a fundamentally new relkind is going to involve some
tweaking of what those principles are.
... This message also does nothing to help the user understand WHY we don't
allow renaming the attributes of his sequence or TOAST table, whereas
the proposed revision does.
I remain unconvinced that the average user cares, or will be able to
extrapolate the message to understand what's supported or not, even
if he does care about the reason for the restriction.
regards, tom lane
On Thu, Dec 30, 2010 at 9:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
I think this thread has worked itself around to where it's entirely
pointless.I understand your frustration, but it's not clear to me that there *is*
any simple solution to this problem. Fundamentally, adding new relkinds
to the system is always going to require running around and looking at a
lot of code to see what's affected; and that goes for the error messages
too. I put no stock at all in the idea that writing a "guiding
principle" in the error messages will avoid anything, because as often
as not, adding a fundamentally new relkind is going to involve some
tweaking of what those principles are.
I think that's true in some cases but not all. The system-generated
attribute names thing actually applies in several cases, and I think
it's pretty cut-and-dried. When you get into something like which
kinds of relations support triggers, that's a lot more arbitrary.
... This message also does nothing to help the user understand WHY we don't
allow renaming the attributes of his sequence or TOAST table, whereas
the proposed revision does.I remain unconvinced that the average user cares, or will be able to
extrapolate the message to understand what's supported or not, even
if he does care about the reason for the restriction.
I'm convinced, but that only makes one of us. I think for now what I
had better do is try to get this SQL/MED patch finished up by
soldiering through this mess rather than trying to fix it. I think
it's going to be kind of ugly, but we haven't got another plan then
we're just going to have to live with the ugliness.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of vie dic 31 02:07:18 -0300 2010:
I think that's true in some cases but not all. The system-generated
attribute names thing actually applies in several cases, and I think
it's pretty cut-and-dried. When you get into something like which
kinds of relations support triggers, that's a lot more arbitrary.
I think part of the problem with the phrase "system-generated attribute
names" is: how are users supposed to figure out what that means, and
what relation types it applies to? It seems entirely non-obvious.
I think for now what I
had better do is try to get this SQL/MED patch finished up by
soldiering through this mess rather than trying to fix it. I think
it's going to be kind of ugly, but we haven't got another plan then
we're just going to have to live with the ugliness.
Perhaps it would make sense to fix the cases for which there is a
consensus, and leave the rest alone for now.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Fri, Dec 31, 2010 at 8:10 AM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
I think for now what I
had better do is try to get this SQL/MED patch finished up by
soldiering through this mess rather than trying to fix it. I think
it's going to be kind of ugly, but we haven't got another plan then
we're just going to have to live with the ugliness.Perhaps it would make sense to fix the cases for which there is a
consensus, and leave the rest alone for now.
Sure. Which cases do we have consensus on?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On tor, 2010-12-30 at 11:03 -0500, Robert Haas wrote:
No, quite the opposite. With the other approach, you needed:
constraints cannot be used on views
constraints cannot be used on composite types
constraints cannot be used on TOAST tables
constraints cannot be used on indexes
constraints cannot be used on foreign tablesWith this, you just need:
constraints can only be used on tables
At the beginning of this thread you said that the error messages should
focus on what you tried to do, not what you could do instead.
Also, in this particular case, the user could very well assume that a
TOAST table or a foreign table is a table.
On tor, 2010-12-30 at 11:49 -0500, Tom Lane wrote:
ISTM there are four things we might potentially want to state in the
error message: the feature/operation you tried to apply, the name of
the object you tried to apply it to, the type of that object, and the
set of object types that the feature/operation will actually work for.
I think the latter should be completely omitted unless it's
exceptionally important.
You can construct pretty silly things down this line:
ERROR: permission denied for relation "x"
ERROR: relation "x" does not exist
vs.
ERROR: you only have permission on relation a, b, c
ERROR: only the following relations exist: a, b, c
On Fri, Dec 31, 2010 at 8:48 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
On tor, 2010-12-30 at 11:03 -0500, Robert Haas wrote:
No, quite the opposite. With the other approach, you needed:
constraints cannot be used on views
constraints cannot be used on composite types
constraints cannot be used on TOAST tables
constraints cannot be used on indexes
constraints cannot be used on foreign tablesWith this, you just need:
constraints can only be used on tables
At the beginning of this thread you said that the error messages should
focus on what you tried to do, not what you could do instead.
Yeah, and I still believe that. I'm having difficulty coming up with
a workable approach, though. It would be simple enough if we could
write:
/* translator: first %s is a feature, second %s is a relation type */
%s cannot be used on %s
...but I think this is likely to cause some translation headaches.
Also, in this particular case, the user could very well assume that a
TOAST table or a foreign table is a table.
There's a limited amount we can do about confused users, but it is
true that the negative phrasing is better for that case.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Le 01/01/2011 06:05, Robert Haas a �crit :
On Fri, Dec 31, 2010 at 8:48 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
On tor, 2010-12-30 at 11:03 -0500, Robert Haas wrote:
No, quite the opposite. With the other approach, you needed:
constraints cannot be used on views
constraints cannot be used on composite types
constraints cannot be used on TOAST tables
constraints cannot be used on indexes
constraints cannot be used on foreign tablesWith this, you just need:
constraints can only be used on tables
At the beginning of this thread you said that the error messages should
focus on what you tried to do, not what you could do instead.Yeah, and I still believe that. I'm having difficulty coming up with
a workable approach, though. It would be simple enough if we could
write:/* translator: first %s is a feature, second %s is a relation type */
%s cannot be used on %s...but I think this is likely to cause some translation headaches.
Actually, this is simply not translatable in some languages. We had the
same issue on pgAdmin, and we resolved this by having quite a big number
of new strings to translate. Harder one time for the translator, but
results in a much better experience for the user.
Also, in this particular case, the user could very well assume that a
TOAST table or a foreign table is a table.There's a limited amount we can do about confused users, but it is
true that the negative phrasing is better for that case.
It's at least better for the translator.
--
Guillaume
http://www.postgresql.fr
http://dalibo.com
On Sat, Jan 1, 2011 at 9:53 AM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:
Le 01/01/2011 06:05, Robert Haas a écrit :
On Fri, Dec 31, 2010 at 8:48 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
On tor, 2010-12-30 at 11:03 -0500, Robert Haas wrote:
No, quite the opposite. With the other approach, you needed:
constraints cannot be used on views
constraints cannot be used on composite types
constraints cannot be used on TOAST tables
constraints cannot be used on indexes
constraints cannot be used on foreign tablesWith this, you just need:
constraints can only be used on tables
At the beginning of this thread you said that the error messages should
focus on what you tried to do, not what you could do instead.Yeah, and I still believe that. I'm having difficulty coming up with
a workable approach, though. It would be simple enough if we could
write:/* translator: first %s is a feature, second %s is a relation type */
%s cannot be used on %s...but I think this is likely to cause some translation headaches.
Actually, this is simply not translatable in some languages. We had the
same issue on pgAdmin, and we resolved this by having quite a big number
of new strings to translate. Harder one time for the translator, but
results in a much better experience for the user.
Is it in any better if we write one string per feature, like this:
constraints cannot be used on %s
triggers cannot be used on %s
...where %s is a plural object type (views, foreign tables, etc.).
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Le 01/01/2011 16:00, Robert Haas a �crit :
On Sat, Jan 1, 2011 at 9:53 AM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:Le 01/01/2011 06:05, Robert Haas a �crit :
On Fri, Dec 31, 2010 at 8:48 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
On tor, 2010-12-30 at 11:03 -0500, Robert Haas wrote:
No, quite the opposite. With the other approach, you needed:
constraints cannot be used on views
constraints cannot be used on composite types
constraints cannot be used on TOAST tables
constraints cannot be used on indexes
constraints cannot be used on foreign tablesWith this, you just need:
constraints can only be used on tables
At the beginning of this thread you said that the error messages should
focus on what you tried to do, not what you could do instead.Yeah, and I still believe that. I'm having difficulty coming up with
a workable approach, though. It would be simple enough if we could
write:/* translator: first %s is a feature, second %s is a relation type */
%s cannot be used on %s...but I think this is likely to cause some translation headaches.
Actually, this is simply not translatable in some languages. We had the
same issue on pgAdmin, and we resolved this by having quite a big number
of new strings to translate. Harder one time for the translator, but
results in a much better experience for the user.Is it in any better if we write one string per feature, like this:
constraints cannot be used on %s
triggers cannot be used on %s...where %s is a plural object type (views, foreign tables, etc.).
If %s was a singular object, it would be an issue for french. But for
plural form, it won't be an issue. Not sure it would be the same in
other languages. IIRC from my student years, german could have an issue
here.
--
Guillaume
http://www.postgresql.fr
http://dalibo.com
On lör, 2011-01-01 at 00:05 -0500, Robert Haas wrote:
Yeah, and I still believe that. I'm having difficulty coming up with
a workable approach, though.
I don't see anything wrong with having 20 or 30 messages of variants of
"foo cannot be used on bar"
without placeholders.
On lör, 2011-01-01 at 10:00 -0500, Robert Haas wrote:
Is it in any better if we write one string per feature, like this:
constraints cannot be used on %s
triggers cannot be used on %s...where %s is a plural object type (views, foreign tables, etc.).
No, this won't work.
On Sat, Jan 1, 2011 at 4:28 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
On lör, 2011-01-01 at 00:05 -0500, Robert Haas wrote:
Yeah, and I still believe that. I'm having difficulty coming up with
a workable approach, though.I don't see anything wrong with having 20 or 30 messages of variants of
"foo cannot be used on bar"
without placeholders.
Well, that's OK with me. It seems a little grotty, but manageably so.
Questions:
1. Should we try to include the name of the object? If so, how?
2. Can we have a variant with an SQL-command-fragment parameter?
%s cannot be used on views
where %s might be CLUSTER, DROP COLUMN, etc.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On lör, 2011-01-01 at 17:21 -0500, Robert Haas wrote:
I don't see anything wrong with having 20 or 30 messages of variants of
"foo cannot be used on bar"
without placeholders.
Well, that's OK with me. It seems a little grotty, but manageably so.
Questions:1. Should we try to include the name of the object? If so, how?
Hmm. There is a bit of a difference in my mind between, say,
constraints cannot be used on sequences
constraint "foo" cannot be used on sequence "bar"
the latter leaving open the question whether some other combination
might work.
2. Can we have a variant with an SQL-command-fragment parameter?
%s cannot be used on views
where %s might be CLUSTER, DROP COLUMN, etc.
That's OK; we do that in several other places.
On Sun, Jan 2, 2011 at 4:45 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
On lör, 2011-01-01 at 17:21 -0500, Robert Haas wrote:
I don't see anything wrong with having 20 or 30 messages of variants of
"foo cannot be used on bar"
without placeholders.
Well, that's OK with me. It seems a little grotty, but manageably so.
Questions:1. Should we try to include the name of the object? If so, how?
Hmm. There is a bit of a difference in my mind between, say,
constraints cannot be used on sequences
constraint "foo" cannot be used on sequence "bar"
the latter leaving open the question whether some other combination
might work.
Yeah, that's no good. Maybe there's a good way to clear things up
with an errdetail(), though I'm having a hard time thinking how to
phrase it.
ERROR: sequence "%s" does not support the requested operation
DETAIL: Constraints are not supported on sequences.
ERROR: constraints are not supported on sequences
DETAIL: "%s" is a sequence.
ERROR: "%s" is a sequence
DETAIL: Constraints and sequences are like water and oil, dude.
2. Can we have a variant with an SQL-command-fragment parameter?
%s cannot be used on views
where %s might be CLUSTER, DROP COLUMN, etc.That's OK; we do that in several other places.
Cool.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of lun ene 03 12:21:44 -0300 2011:
Yeah, that's no good. Maybe there's a good way to clear things up
with an errdetail(), though I'm having a hard time thinking how to
phrase it.ERROR: sequence "%s" does not support the requested operation
DETAIL: Constraints are not supported on sequences.
This seems backwards to me: the "detail" is more general than the main
message.
ERROR: constraints are not supported on sequences
DETAIL: "%s" is a sequence.
This one seems good -- the detail message gives the detail.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support