Re: pgsql-server/src/backend commands/typecmds.c e ... - Mailing list pgsql-committers

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