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/[email protected]
Best regards,
Alexander
