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

Hello Tom,

06.09.2026 22:07, Tom Lane wrote:
> 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.

Thank you for the clarification! Will change this way.

> @@ -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?

Yeah, I considered this, but the same errmsg in
AddRelationNotNullConstraints() uses ERRCODE_SYNTAX_ERROR. I found it
more consistent to emit the same code. Maybe it would make do the opposite
-- change the errcode in AddRelationNotNullConstraints() (there are two
instances there, though)... Moreover, ERRCODE_SYNTAX_ERROR is chosen for
many other similar messages in transformColumnDefinition(), so I'm not
sure if it makes sense to change all of those or just report "syntax error"
for consistency while keeping the patch focused.

> @@ -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).

Thank you for the suggestion! Will change the code and add the protection.

> @@ -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

Yeah, I have this one in my collection too, but started from internal
errors reported previously -- that one at [1], it is reachable with,
e.g.:
SELECT pg_catalog.interval('1 second', 0);
ERROR:  XX000: unrecognized interval typmod: 0
LOCATION:  AdjustIntervalForTypmod, timestamp.c:1492

> 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.

Will check them and include into this patch.

> @@ -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.

Yes, I'm not aware of cases where one errmsg is issued with different
codes (probably there may be such cases with very generic messages), but
I'd stick to reusing existing code if it's not too wrong.

[1] https://www.postgresql.org/message-id/20230213135053.GZ1653@telsasoft.com

Best regards,
Alexander