Re: Internal error codes triggered by regression tests and user queries, take 2

Alexander Lakhin <exclusion(at)gmail(dot)com> writes:
> Thank you for having a look! Please find v2 patch with 7 approved changes
> attached.

Thanks for working on this cleanup, but I have a few quibbles:

@@ -4314,7 +4314,9 @@ getObjectDescription(const ObjectAddress *object, bool missing_ok)
}

default:
- elog(ERROR, "unsupported object class: %u", object->classId);
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("unsupported object class: %u", object->classId)));
}

I think we mostly use ERRCODE_WRONG_OBJECT_TYPE for cases where a SQL
object exists but is the wrong kind for the command (eg, a table for
a command that operates on indexes). Here that's not the case;
there cannot be any relevant object. The only access path that
I know of is something like

regression=# select pg_describe_object(34,56,0);
ERROR: unsupported object class: 34

so I'd be kind of inclined to use ERRCODE_INVALID_PARAMETER_VALUE.

@@ -794,8 +794,10 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
if (constraint->conname &&
notnull_constraint->conname &&
strcmp(notnull_constraint->conname, constraint->conname) != 0)
- elog(ERROR, "conflicting not-null constraint names \"%s\" and \"%s\"",
- notnull_constraint->conname, constraint->conname);
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("conflicting not-null constraint names \"%s\" and \"%s\"",
+ notnull_constraint->conname, constraint->conname));

This does not seem like a "syntax error". Perhaps
ERRCODE_INVALID_OBJECT_DEFINITION would serve?

@@ -996,7 +996,9 @@ acldefault_sql(PG_FUNCTION_ARGS)
objtype = OBJECT_TYPE;
break;
default:
- elog(ERROR, "unrecognized object type abbreviation: %c", objtypec);
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("unrecognized object type abbreviation: %c", objtypec)));
}

As above, I think ERRCODE_INVALID_PARAMETER_VALUE is better suited.
Also, if we're going to take this seriously as a reachable error,
we need to protect against producing invalidly-encoded output
in case objtypec has the high bit set (compare ce6bf3cd1).

@@ -1489,7 +1489,9 @@ AdjustIntervalForTypmod(Interval *interval, int32 typmod,
/* fractional-second rounding will be dealt with below */
}
else
- elog(ERROR, "unrecognized interval typmod: %d", typmod);
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("unrecognized interval typmod: %d", typmod)));

Hmm, is this actually a user-reachable case? The place that I think
is reachable is over here:

regression=# select format_type('interval'::regtype, 52);
ERROR: invalid INTERVAL typmod: 0x34
regression=# \errverbose
ERROR: XX000: invalid INTERVAL typmod: 0x34
LOCATION: intervaltypmodout, timestamp.c:1189

We'd be well advised to make those two messages more consistent,
whether they're worth translating or not. Also I guess a sweep
for other "unreachable" errors in typmodout functions might be
useful.

@@ -393,7 +393,9 @@ comp_option : '#' K_OPTION K_DUMP
else if (strcmp($3, "off") == 0)
plpgsql_curr_compile->print_strict_params = false;
else
- elog(ERROR, "unrecognized print_strict_params option %s", $3);
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("unrecognized %s option \"%s\"", "print_strict_params", $3)));
}
| '#' K_VARIABLE_CONFLICT K_ERROR
{

There's an argument for ERRCODE_INVALID_PARAMETER_VALUE here too,
perhaps. But I see that all the other uses of this errmsg string
use ERRCODE_SYNTAX_ERROR, so maybe it's fine as you have it.

regards, tom lane