Hello Michael,
31.08.2026 09:15, Michael Paquier wrote:
Not all of them are. See below for details.
Thank you for having a look! Please find v2 patch with 7 approved changes
attached.
Looking (finally) at the list you have sent in your patch, I can get
on board for:
- getObjectDescription(), can be queried with a function officially
documented.
I kept the message as-is, as it's used in 3 other places, with elog(ERROR), but
still...
- ATPrepAddPrimaryKey(). This one is a bug to me, reachable with SQL.
already translatable
- transformColumnDefinition(). Also a bug to me.
already translatable, thanks to AddRelationNotNullConstraints(), I
borrowed ERRCODE_SYNTAX_ERROR from it
- pg_get_shmem_allocations_numa. Bug.
a unique message, looks good enough to be user-visible
- acldefault() is documented. Seems worth fixing for correctness,
documented function.
the message "unrecognized object type abbreviation: %c" is unique, a
closest one is "unrecognized object type \"%s\"", but I don't find it
suitable here
- AdjustIntervalForTypmod(). Documented feature.
the message is unique, looks good too
- The pl_gram.y one looks like a defect to me by not assigning an
error code.
reused the closest translatable message "unrecognized %s option \"%s\""
But not for these:
- relation_open(), incorrect internal state from the caller.
- try_relation_open(), incorrect internal state from the caller.
coerce_type(), also an internal state. unknownin() is also an
- "internal" input, non-documented SQL function.
- check_float8_array(). Your case involves float8_regr_intercept(),
which is a final aggregate function. It is not documented, and should
never really be called directly.
- get_range_io_data(). Cannot get excited about this one, either, as
your case involves the direct call of an input function. Can this
happen with more patterns?
- test_pglz_decompress() is test code, no need to worry about it.
I agree, let's leave them aside until other (documented) ways to reach
them found.
The other one is the unclear definition of internal/XX000 errors in
principle. As far as I can see, it varies from "all the errors that have
no specific code assigned" to "critical errors, similar to failed asserts"
(and probably a substitute for asserts in production). And as Andres (and
I too) find it useful to search for XX000 in production logs, it means
they are interpreted per the second definition. In this case, it makes
sense to define missing codes for cases which are not worth paying
attention to in production, e.g. "#print_strict_params XXX". Moreover, if
the second definition (XX000 error ~ failed assert) is going to be
accepted, it seems sensible to have no such errors produced during
regression tests (like no assertion failures).
I think that I would draw a line depending on the documentation. If
a function is documented as usable, or callable through a view, it
sounds fair to me to assume that a user gets an error code that fits
with the incorrect behavior provided as input argument. Input
function for types, test code, direct function calls to embedded
facilities like aggregates, or anything like that, not documented, is
not worth bothering. The fact that none of them is documented is an
argument good enough for me to state that we don't really expect users
to use these this way.
The important piece to keep in mind is that non-internal ereport()
calls mean translation. There is no point in providing translation
for states that we expect users to never see in the field, and the
docs define what we make user-visible.
Thank you for sharing your point of view! I'll review my collection in
light of it.
Best regards,
Alexander
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 703754a8123..cf6f4d866e4 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -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)));
}
/* an empty buffer is equivalent to no object found */
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index fd144d783d9..a3865c02e20 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -9631,6 +9631,7 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd,
tup = findNotNullConstraint(childrelid, strVal(column));
if (!tup)
ereport(ERROR,
+ errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("column \"%s\" of table \"%s\" is not marked NOT NULL",
strVal(column), get_rel_name(childrelid)));
/* verify it's good enough */
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index d83616a8507..360bcbb2cbd 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -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));
if (notnull_constraint->is_no_inherit != constraint->is_no_inherit)
ereport(ERROR,
diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c
index f971ee24192..f78c34daee1 100644
--- a/src/backend/storage/ipc/shmem.c
+++ b/src/backend/storage/ipc/shmem.c
@@ -1210,7 +1210,9 @@ pg_get_shmem_allocations_numa(PG_FUNCTION_ARGS)
Size *nodes;
if (pg_numa_init() == -1)
- elog(ERROR, "libnuma initialization failed or NUMA is not supported on this platform");
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("libnuma initialization failed or NUMA is not supported on this platform")));
InitMaterializedSRF(fcinfo, 0);
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 25cd5d0296b..2385c1ebbe9 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -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)));
}
PG_RETURN_ACL_P(acldefault(objtype, owner));
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 9c17ba2f905..ab86a03280e 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -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)));
/* Need to adjust sub-second precision? */
if (precision != INTERVAL_FULL_PRECISION)
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 5e14a2d7302..37785f58cc2 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -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
{
