Thread: pgsql-server/src/backend commands/typecmds.c e ...
pgsql-server/src/backend commands/typecmds.c e ...
From
petere@svr1.postgresql.org (Peter Eisentraut - PostgreSQL)
Date:
CVSROOT: /cvsroot
Module name: pgsql-server
Changes by: petere@svr1.postgresql.org 03/09/09 20:22:21
Modified files:
src/backend/commands: typecmds.c
src/backend/executor: execQual.c functions.c
src/backend/parser: gram.y
src/backend/tcop: utility.c
src/backend/utils/adt: acl.c array_userfuncs.c pgstatfuncs.c
ri_triggers.c ruleutils.c sets.c varbit.c
src/backend/utils/fmgr: funcapi.c
Log message:
Some "feature not supported" errors are better syntax errors, because the
feature they complain about isn't a feature or cannot be implemented without
definitional changes.
petere@svr1.postgresql.org (Peter Eisentraut - PostgreSQL) writes:
> Modified files:
> src/backend/commands: typecmds.c
> src/backend/executor: execQual.c functions.c
> src/backend/parser: gram.y
> src/backend/tcop: utility.c
> src/backend/utils/adt: acl.c array_userfuncs.c pgstatfuncs.c
> ri_triggers.c ruleutils.c sets.c varbit.c
> src/backend/utils/fmgr: funcapi.c
> Log message:
> Some "feature not supported" errors are better syntax errors, because the
> feature they complain about isn't a feature or cannot be implemented without
> definitional changes.
I looked over this change, and right offhand I don't agree with any of
it. None of the errors are "syntax" errors (if they were, gram.y would
have barfed) and the textual error message changes generally seem to be
trending in a less not more helpful direction. What is your rationale
for this, please?
regards, tom lane
Tom Lane writes:
> I looked over this change, and right offhand I don't agree with any of
> it. None of the errors are "syntax" errors (if they were, gram.y would
> have barfed) and the textual error message changes generally seem to be
> trending in a less not more helpful direction. What is your rationale
> for this, please?
A syntax error is a violation of the syntax rules of the SQL standard or a
hypothetical document describing the PostgreSQL feature set using the same
layout. Syntax rules can be evaluated without knowing the state of the
database. Not all syntax errors can be caught in gram.y; see below.
The way I see this is that "feature not supported" appeared to have been
used as a catch-all thinking "maybe someday this will work". But it's
easy to see that this definition has no upper bound. There are certain
things that we have a pretty clear idea about how it will work, and there
are a few stubs already in place, and we make a bit of a promise that
someday it will appear in the shape that is apparent today.
In detail:
> diff -cr ../cvs-pgsql/src/backend/commands/typecmds.c ./src/backend/commands/typecmds.c
> *** ../cvs-pgsql/src/backend/commands/typecmds.c Tue Aug 19 22:59:37 2003
> --- ./src/backend/commands/typecmds.c Wed Sep 10 00:42:55 2003
> ***************
> *** 594,601 ****
> /* Check for unsupported constraint types */
> if (IsA(newConstraint, FkConstraint))
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> ! errmsg("FOREIGN KEY constraints not supported for domains")));
>
> /* otherwise it should be a plain Constraint */
> if (!IsA(newConstraint, Constraint))
> --- 594,601 ----
> /* Check for unsupported constraint types */
> if (IsA(newConstraint, FkConstraint))
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> ! errmsg("foreign key constraints not possible for domains")));
>
> /* otherwise it should be a plain Constraint */
> if (!IsA(newConstraint, Constraint))
Foreign keys on domains aren't a feature that is a reasonable prospect.
The only reason this is caught here is that someone was lazy in gram.y
splitting up the contraint rules for tables and domains properly.
> ***************
> *** 672,685 ****
> */
> case CONSTR_UNIQUE:
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> ! errmsg("UNIQUE constraints not supported for domains")));
> break;
>
> case CONSTR_PRIMARY:
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> ! errmsg("PRIMARY KEY constraints not supported for domains")));
> break;
>
> case CONSTR_ATTR_DEFERRABLE:
> --- 672,685 ----
> */
> case CONSTR_UNIQUE:
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> ! errmsg("unique constraints not possible for domains")));
> break;
>
> case CONSTR_PRIMARY:
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> ! errmsg("primary key constraints not possible for domains")));
> break;
>
> case CONSTR_ATTR_DEFERRABLE:
The reason I lowercased these things is because in English you don't write
things in uppercase as a rule. -- But they are key words! -- Well, in
other places it was yelling "FOREIGN KEY constraints are not supported",
but maybe the user was writing it as a column constraint, so FOREIGN KEY
appears nowhere. And what are the uppercase things telling the
translator?
> ***************
> *** 688,694 ****
> case CONSTR_ATTR_IMMEDIATE:
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> ! errmsg("deferrability constraints not supported for domains")));
> break;
>
> default:
> --- 688,694 ----
> case CONSTR_ATTR_IMMEDIATE:
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> ! errmsg("specifying constraint deferrability not supported for domains")));
> break;
>
> default:
That was clearly a miswording.
> ***************
> *** 1453,1460 ****
> /* Check for unsupported constraint types */
> if (IsA(newConstraint, FkConstraint))
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> ! errmsg("FOREIGN KEY constraints not supported for domains")));
>
> /* otherwise it should be a plain Constraint */
> if (!IsA(newConstraint, Constraint))
> --- 1453,1460 ----
> /* Check for unsupported constraint types */
> if (IsA(newConstraint, FkConstraint))
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> ! errmsg("foreign key constraints not possible for domains")));
>
> /* otherwise it should be a plain Constraint */
> if (!IsA(newConstraint, Constraint))
See above.
> ***************
> *** 1465,1497 ****
>
> switch (constr->contype)
> {
> - case CONSTR_DEFAULT:
> - ereport(ERROR,
> - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> - errmsg("use ALTER DOMAIN .. SET DEFAULT instead")));
> - break;
> -
> - case CONSTR_NOTNULL:
> - case CONSTR_NULL:
> - ereport(ERROR,
> - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> - errmsg("use ALTER DOMAIN .. [ SET | DROP ] NOT NULL instead")));
> - break;
> -
> case CONSTR_CHECK:
> /* processed below */
> break;
>
> case CONSTR_UNIQUE:
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> ! errmsg("UNIQUE constraints not supported for domains")));
> break;
>
> case CONSTR_PRIMARY:
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> ! errmsg("PRIMARY KEY constraints not supported for domains")));
> break;
>
> case CONSTR_ATTR_DEFERRABLE:
> --- 1465,1484 ----
>
> switch (constr->contype)
> {
> case CONSTR_CHECK:
> /* processed below */
> break;
>
> case CONSTR_UNIQUE:
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> ! errmsg("unique constraints not possible for domains")));
> break;
>
> case CONSTR_PRIMARY:
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> ! errmsg("primary key constraints not possible for domains")));
> break;
>
> case CONSTR_ATTR_DEFERRABLE:
The things I deleted at the top are not possible because gram.y won't
construct such things. There's an internal-error catch-all at the bottom.
> ***************
> *** 1500,1506 ****
> case CONSTR_ATTR_IMMEDIATE:
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> ! errmsg("deferrability constraints not supported for domains")));
> break;
>
> default:
> --- 1487,1493 ----
> case CONSTR_ATTR_IMMEDIATE:
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> ! errmsg("specifying constraint deferrability not supported for domains")));
> break;
>
> default:
See above.
> diff -cr ../cvs-pgsql/src/backend/executor/execQual.c ./src/backend/executor/execQual.c
> *** ../cvs-pgsql/src/backend/executor/execQual.c Tue Aug 19 22:59:40 2003
> --- ./src/backend/executor/execQual.c Wed Sep 10 00:24:07 2003
> ***************
> *** 713,719 ****
> *isDone = ExprEndResult;
> else
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("set-valued function called in context that cannot accept a set")));
> return (Datum) 0;
> }
> --- 713,719 ----
> *isDone = ExprEndResult;
> else
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("set-valued function called in context that cannot accept a set")));
> return (Datum) 0;
> }
This seems kind of obvious. How would a "supported feature" that allows
set-valued functions to be called in contexts that cannot accept a set
look like. By this standard, everything is an unsupported feature.
> ***************
> *** 757,763 ****
> */
> if (isDone == NULL)
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("set-valued function called in context that cannot accept a set")));
>
> /*
> --- 757,763 ----
> */
> if (isDone == NULL)
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("set-valued function called in context that cannot accept a set")));
>
> /*
> ***************
> *** 944,950 ****
> /* We don't allow sets in the arguments of the table function */
> if (argDone != ExprSingleResult)
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("set-valued function called in context that cannot accept a set")));
>
> /*
> --- 944,950 ----
> /* We don't allow sets in the arguments of the table function */
> if (argDone != ExprSingleResult)
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("set-valued function called in context that cannot accept a set")));
>
> /*
> ***************
> *** 2955,2961 ****
> /* We have a set-valued expression in the tlist */
> if (isDone == NULL)
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("set-valued function called in context that cannot accept a set")));
> if (itemIsDone[resind] == ExprMultipleResult)
> {
> --- 2955,2961 ----
> /* We have a set-valued expression in the tlist */
> if (isDone == NULL)
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("set-valued function called in context that cannot accept a set")));
> if (itemIsDone[resind] == ExprMultipleResult)
> {
> diff -cr ../cvs-pgsql/src/backend/executor/functions.c ./src/backend/executor/functions.c
> *** ../cvs-pgsql/src/backend/executor/functions.c Tue Aug 19 22:59:40 2003
> --- ./src/backend/executor/functions.c Wed Sep 10 00:24:58 2003
> ***************
> *** 574,580 ****
> rsi->isDone = ExprEndResult;
> else
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("set-valued function called in context that cannot accept a set")));
> fcinfo->isnull = true;
> result = (Datum) 0;
> --- 574,580 ----
> rsi->isDone = ExprEndResult;
> else
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("set-valued function called in context that cannot accept a set")));
> fcinfo->isnull = true;
> result = (Datum) 0;
> ***************
> *** 613,619 ****
> rsi->isDone = ExprMultipleResult;
> else
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("set-valued function called in context that cannot accept a set")));
>
> /*
> --- 613,619 ----
> rsi->isDone = ExprMultipleResult;
> else
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("set-valued function called in context that cannot accept a set")));
>
> /*
See above.
> diff -cr ../cvs-pgsql/src/backend/parser/gram.y ./src/backend/parser/gram.y
> *** ../cvs-pgsql/src/backend/parser/gram.y Sat Sep 6 16:01:51 2003
> --- ./src/backend/parser/gram.y Wed Sep 10 00:25:54 2003
> ***************
> *** 4610,4616 ****
> {
> /* Disabled because it was too confusing, bjm 2002-02-18 */
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("LIMIT #,# syntax is not supported"),
> errhint("Use separate LIMIT and OFFSET clauses.")));
> }
> --- 4610,4616 ----
> {
> /* Disabled because it was too confusing, bjm 2002-02-18 */
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("LIMIT #,# syntax is not supported"),
> errhint("Use separate LIMIT and OFFSET clauses.")));
> }
It's syntax, and it's an error. We cannot upgrade every syntax that we
deprecate to an unsupported feature. If we removed that warning and let
the grammar itself handle it, it would become a syntax error anyway.
> diff -cr ../cvs-pgsql/src/backend/tcop/utility.c ./src/backend/tcop/utility.c
> *** ../cvs-pgsql/src/backend/tcop/utility.c Tue Aug 19 23:00:08 2003
> --- ./src/backend/tcop/utility.c Wed Sep 10 00:39:48 2003
> ***************
> *** 630,636 ****
> */
> switch (stmt->subtype)
> {
> ! case 'T': /* ALTER COLUMN DEFAULT */
>
> /*
> * Recursively alter column default for table and,
> --- 630,636 ----
> */
> switch (stmt->subtype)
> {
> ! case 'T': /* ALTER DOMAIN DEFAULT */
>
> /*
> * Recursively alter column default for table and,
> ***************
> *** 639,649 ****
> AlterDomainDefault(stmt->typename,
> stmt->def);
> break;
> ! case 'N': /* ALTER COLUMN DROP NOT NULL */
> AlterDomainNotNull(stmt->typename,
> false);
> break;
> ! case 'O': /* ALTER COLUMN SET NOT NULL */
> AlterDomainNotNull(stmt->typename,
> true);
> break;
> --- 639,649 ----
> AlterDomainDefault(stmt->typename,
> stmt->def);
> break;
> ! case 'N': /* ALTER DOMAIN DROP NOT NULL */
> AlterDomainNotNull(stmt->typename,
> false);
> break;
> ! case 'O': /* ALTER DOMAIN SET NOT NULL */
> AlterDomainNotNull(stmt->typename,
> true);
> break;
copy and paste laziness...
> diff -cr ../cvs-pgsql/src/backend/utils/adt/acl.c ./src/backend/utils/adt/acl.c
> *** ../cvs-pgsql/src/backend/utils/adt/acl.c Tue Aug 19 23:00:09 2003
> --- ./src/backend/utils/adt/acl.c Wed Sep 10 00:18:48 2003
> ***************
> *** 846,852 ****
> else if (u_grantee != 0 && g_grantee != 0)
> {
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("cannot specify both user and group")));
> }
> else if (u_grantee != 0)
> --- 846,852 ----
> else if (u_grantee != 0 && g_grantee != 0)
> {
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("cannot specify both user and group")));
> }
> else if (u_grantee != 0)
This was never a feature, it was a definitional impossibility.
> diff -cr ../cvs-pgsql/src/backend/utils/adt/array_userfuncs.c ./src/backend/utils/adt/array_userfuncs.c
> *** ../cvs-pgsql/src/backend/utils/adt/array_userfuncs.c Tue Aug 19 23:00:09 2003
> --- ./src/backend/utils/adt/array_userfuncs.c Wed Sep 10 00:19:22 2003
> ***************
> *** 95,102 ****
> indx = 1;
> else
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> ! errmsg("input must be empty or one-dimensional array")));
>
> /*
> * We arrange to look up info about element type only once per series
> --- 95,102 ----
> indx = 1;
> else
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> ! errmsg("argument must be empty or one-dimensional array")));
>
> /*
> * We arrange to look up info about element type only once per series
Push works on a stack, and a stack is always one-dimensional. It's never
going to be supported under the label "push". Maybe a separate error
along the lines of "cardinality error" would be appropriate.
> diff -cr ../cvs-pgsql/src/backend/utils/adt/pgstatfuncs.c ./src/backend/utils/adt/pgstatfuncs.c
> *** ../cvs-pgsql/src/backend/utils/adt/pgstatfuncs.c Tue Aug 19 23:00:17 2003
> --- ./src/backend/utils/adt/pgstatfuncs.c Wed Sep 10 00:26:35 2003
> ***************
> *** 187,193 ****
> if (fcinfo->resultinfo == NULL ||
> !IsA(fcinfo->resultinfo, ReturnSetInfo))
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("set-valued function called in context that "
> "cannot accept a set")));
>
> --- 187,193 ----
> if (fcinfo->resultinfo == NULL ||
> !IsA(fcinfo->resultinfo, ReturnSetInfo))
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("set-valued function called in context that "
> "cannot accept a set")));
>
See above.
> diff -cr ../cvs-pgsql/src/backend/utils/adt/ri_triggers.c ./src/backend/utils/adt/ri_triggers.c
> *** ../cvs-pgsql/src/backend/utils/adt/ri_triggers.c Tue Aug 19 23:00:18 2003
> --- ./src/backend/utils/adt/ri_triggers.c Wed Sep 10 00:45:07 2003
> ***************
> *** 3009,3015 ****
>
> if (spi_err)
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("referential integrity query on \"%s\" from constraint \"%s\" on \"%s\" gave unexpected
result",
> RelationGetRelationName(pk_rel),
> constrname,
> --- 3009,3015 ----
>
> if (spi_err)
> ereport(ERROR,
> ! (errcode(ERRCODE_INTERNAL_ERROR),
> errmsg("referential integrity query on \"%s\" from constraint \"%s\" on \"%s\" gave unexpected
result",
> RelationGetRelationName(pk_rel),
> constrname,
I don't see a "feature" here.
> diff -cr ../cvs-pgsql/src/backend/utils/adt/ruleutils.c ./src/backend/utils/adt/ruleutils.c
> *** ../cvs-pgsql/src/backend/utils/adt/ruleutils.c Tue Aug 19 23:00:19 2003
> --- ./src/backend/utils/adt/ruleutils.c Wed Sep 10 00:29:33 2003
> ***************
> *** 1098,1105 ****
> }
> default:
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> ! errmsg("unsupported constraint type \"%c\"",
> conForm->contype)));
> break;
> }
> --- 1098,1105 ----
> }
> default:
> ereport(ERROR,
> ! (errcode(ERRCODE_INTERNAL_ERROR),
> ! errmsg("invalid constraint type \"%c\"",
> conForm->contype)));
> break;
> }
This error speaks of bogus values in a system table.
> diff -cr ../cvs-pgsql/src/backend/utils/adt/sets.c ./src/backend/utils/adt/sets.c
> *** ../cvs-pgsql/src/backend/utils/adt/sets.c Tue Aug 19 23:00:20 2003
> --- ./src/backend/utils/adt/sets.c Wed Sep 10 00:29:53 2003
> ***************
> *** 203,209 ****
> rsi->isDone = isDone;
> else
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("set-valued function called in context that "
> "cannot accept a set")));
> }
> --- 203,209 ----
> rsi->isDone = isDone;
> else
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("set-valued function called in context that "
> "cannot accept a set")));
> }
See above.
> diff -cr ../cvs-pgsql/src/backend/utils/adt/varbit.c ./src/backend/utils/adt/varbit.c
> *** ../cvs-pgsql/src/backend/utils/adt/varbit.c Tue Aug 19 23:00:20 2003
> --- ./src/backend/utils/adt/varbit.c Wed Sep 10 00:44:13 2003
> ***************
> *** 901,907 ****
> bitlen2 = VARBITLEN(arg2);
> if (bitlen1 != bitlen2)
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("cannot AND bit strings of different sizes")));
>
> len = VARSIZE(arg1);
> --- 901,907 ----
> bitlen2 = VARBITLEN(arg2);
> if (bitlen1 != bitlen2)
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("cannot AND bit strings of different sizes")));
>
> len = VARSIZE(arg1);
> ***************
> *** 942,948 ****
> bitlen2 = VARBITLEN(arg2);
> if (bitlen1 != bitlen2)
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("cannot OR bit strings of different sizes")));
> len = VARSIZE(arg1);
> result = (VarBit *) palloc(len);
> --- 942,948 ----
> bitlen2 = VARBITLEN(arg2);
> if (bitlen1 != bitlen2)
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("cannot OR bit strings of different sizes")));
> len = VARSIZE(arg1);
> result = (VarBit *) palloc(len);
> ***************
> *** 988,994 ****
> bitlen2 = VARBITLEN(arg2);
> if (bitlen1 != bitlen2)
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("cannot XOR bit strings of different sizes")));
>
> len = VARSIZE(arg1);
> --- 988,994 ----
> bitlen2 = VARBITLEN(arg2);
> if (bitlen1 != bitlen2)
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("cannot XOR bit strings of different sizes")));
>
> len = VARSIZE(arg1);
We once decided (for better or worse) that bit string operations would
only be defined for certain operators. It's not that under the current
design plan we are trying to get better here. We simply decided that
certain combinations of input are invalid.
> diff -cr ../cvs-pgsql/src/backend/utils/fmgr/funcapi.c ./src/backend/utils/fmgr/funcapi.c
> *** ../cvs-pgsql/src/backend/utils/fmgr/funcapi.c Tue Aug 19 23:00:22 2003
> --- ./src/backend/utils/fmgr/funcapi.c Wed Sep 10 00:24:32 2003
> ***************
> *** 34,40 ****
> */
> if (fcinfo->resultinfo == NULL || !IsA(fcinfo->resultinfo, ReturnSetInfo))
> ereport(ERROR,
> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("set-valued function called in context that cannot accept a set")));
>
> if (fcinfo->flinfo->fn_extra == NULL)
> --- 34,40 ----
> */
> if (fcinfo->resultinfo == NULL || !IsA(fcinfo->resultinfo, ReturnSetInfo))
> ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("set-valued function called in context that cannot accept a set")));
>
> if (fcinfo->flinfo->fn_extra == NULL)
Man, this really should be centralized somewhere. :-)
--
Peter Eisentraut peter_e@gmx.net
Peter Eisentraut <peter_e@gmx.net> writes:
> The way I see this is that "feature not supported" appeared to have been
> used as a catch-all thinking "maybe someday this will work".
Well, yes, but your notion of how close to working it has to be to be
considered an unsupported feature rather than a syntax error seems a
little harsh ;-)
> The only reason this is caught here is that someone was lazy in gram.y
> splitting up the contraint rules for tables and domains properly.
Okay, that's fair. I'll subside on most of the rest of these too,
except for
>> ereport(ERROR,
>> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>> errmsg("set-valued function called in context that cannot accept a set")));
>> ereport(ERROR,
>> ! (errcode(ERRCODE_SYNTAX_ERROR),
>> errmsg("set-valued function called in context that cannot accept a set")));
> This seems kind of obvious. How would a "supported feature" that allows
> set-valued functions to be called in contexts that cannot accept a set
> look like.
The aspect of it that's unsupported is that the particular context
(which in this case really means a particular caller of ExecEvalExpr)
isn't prepared to deal with a set-valued result. It seems entirely
likely to me that we might someday improve some of those callers to
allow sets; that's why I marked this as an unsupported feature.
Syntax error does not seem appropriate. The same goes for the other
occurrences of the same error string.
> By this standard, everything is an unsupported feature.
ISTM that by your standard, everything is a syntax error ;-).
"Syntax error" is a sufficiently generic classification that I'd prefer
not to use it when there is anything more specific available.
>> ereport(ERROR,
>> ! (errcode(ERRCODE_SYNTAX_ERROR),
>> errmsg("cannot specify both user and group")));
>> }
> This was never a feature, it was a definitional impossibility.
Mph, maybe that should go back to being an elog (ie, internal error).
Is it possible for control to get here?
>> ereport(ERROR,
>> ! (errcode(ERRCODE_SYNTAX_ERROR),
>> ! errmsg("argument must be empty or one-dimensional array")));
> Push works on a stack, and a stack is always one-dimensional. It's never
> going to be supported under the label "push". Maybe a separate error
> along the lines of "cardinality error" would be appropriate.
This is certainly not a syntax error; it belongs in the data-exception
SQLSTATE category. (AFAICT, "cardinality violation" is supposed to be
reserved for wrong numbers of *rows* in table results, which this
isn't.) ARRAY_SUBSCRIPT_ERROR would be fine with me; I think we use
that in other contexts where the size of an array isn't right.
>> ! (errcode(ERRCODE_INTERNAL_ERROR),
>> ! errmsg("invalid constraint type \"%c\"",
>> conForm->contype)));
> This error speaks of bogus values in a system table.
I'd suggest reverting such things to elog() style, so that translators
don't have to deal with those messages.
>> ereport(ERROR,
>> ! (errcode(ERRCODE_SYNTAX_ERROR),
>> errmsg("cannot XOR bit strings of different sizes")));
>>
>> len = VARSIZE(arg1);
> We once decided (for better or worse) that bit string operations would
> only be defined for certain operators.
These again should be data errors not syntax errors. Perhaps
STRING_DATA_LENGTH_MISMATCH? The SQL spec seems to use "string" to
cover both character and bit strings in some places, so it seems
reasonable to use that SQLSTATE for this.
regards, tom lane
On Wed, Sep 10, 2003 at 02:40:30PM -0400, Tom Lane wrote:
> Peter Eisentraut <peter_e@gmx.net> writes:
> >> ! (errcode(ERRCODE_INTERNAL_ERROR),
> >> ! errmsg("invalid constraint type \"%c\"",
> >> conForm->contype)));
>
> > This error speaks of bogus values in a system table.
>
> I'd suggest reverting such things to elog() style, so that translators
> don't have to deal with those messages.
I was going to suggest that (I translated this very same message two
days ago).
Also, I was going to ask when/if will we have a "string freeze" or some
such - has it been considered?
--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"La realidad se compone de muchos sueños, todos ellos diferentes,
pero en cierto aspecto, parecidos..." (Yo, hablando de sueños eróticos)
Tom Lane writes:
> >> ereport(ERROR,
> >> ! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> >> errmsg("set-valued function called in context that cannot accept a set")));
>
> >> ereport(ERROR,
> >> ! (errcode(ERRCODE_SYNTAX_ERROR),
> >> errmsg("set-valued function called in context that cannot accept a set")));
>
> > This seems kind of obvious. How would a "supported feature" that allows
> > set-valued functions to be called in contexts that cannot accept a set
> > look like.
>
> The aspect of it that's unsupported is that the particular context
> (which in this case really means a particular caller of ExecEvalExpr)
> isn't prepared to deal with a set-valued result. It seems entirely
> likely to me that we might someday improve some of those callers to
> allow sets; that's why I marked this as an unsupported feature.
> Syntax error does not seem appropriate. The same goes for the other
> occurrences of the same error string.
Changed back to unsupported feature.
> >> ereport(ERROR,
> >> ! (errcode(ERRCODE_SYNTAX_ERROR),
> >> errmsg("cannot specify both user and group")));
> >> }
>
> > This was never a feature, it was a definitional impossibility.
>
> Mph, maybe that should go back to being an elog (ie, internal error).
> Is it possible for control to get here?
Yes, it happens when you call the function as makeaclitem(0, 0, ...).
I've changed it to DATA_EXCEPTION. (A more specific code doesn't seem
worthwhile, because this function is mainly for information schema
support.)
> >> ereport(ERROR,
> >> ! (errcode(ERRCODE_SYNTAX_ERROR),
> >> ! errmsg("argument must be empty or one-dimensional array")));
>
> > Push works on a stack, and a stack is always one-dimensional. It's never
> > going to be supported under the label "push". Maybe a separate error
> > along the lines of "cardinality error" would be appropriate.
>
> This is certainly not a syntax error; it belongs in the data-exception
> SQLSTATE category. (AFAICT, "cardinality violation" is supposed to be
> reserved for wrong numbers of *rows* in table results, which this
> isn't.) ARRAY_SUBSCRIPT_ERROR would be fine with me; I think we use
> that in other contexts where the size of an array isn't right.
That would be inappropriate, because it's the dimension, not the size.
In fact, you can view our multidimensional arrays as arrays of arrays in
SQL, so it really boils down to a run-time type mismatch. I've made it
DATA_EXCEPTION for now.
> >> ! (errcode(ERRCODE_INTERNAL_ERROR),
> >> ! errmsg("invalid constraint type \"%c\"",
> >> conForm->contype)));
>
> > This error speaks of bogus values in a system table.
>
> I'd suggest reverting such things to elog() style, so that translators
> don't have to deal with those messages.
Done.
> >> ereport(ERROR,
> >> ! (errcode(ERRCODE_SYNTAX_ERROR),
> >> errmsg("cannot XOR bit strings of different sizes")));
> >>
> >> len = VARSIZE(arg1);
>
> > We once decided (for better or worse) that bit string operations would
> > only be defined for certain operators.
>
> These again should be data errors not syntax errors. Perhaps
> STRING_DATA_LENGTH_MISMATCH? The SQL spec seems to use "string" to
> cover both character and bit strings in some places, so it seems
> reasonable to use that SQLSTATE for this.
Done. In fact, the only standard use I see for "string data, length
mismatch" is for bit strings.
--
Peter Eisentraut peter_e@gmx.net