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

From: Alexander Lakhin <exclusion(at)gmail(dot)com>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Andres Freund <andres(at)anarazel(dot)de>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Justin Pryzby <pryzby(at)telsasoft(dot)com>
Subject: Re: Internal error codes triggered by regression tests and user queries, take 2
Date: 2026-09-05 08:00:00
Message-ID: 91a1b0ab-2c6c-4471-b7a4-ee8ee8aed64e@gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
v2-Define-missing-errcodes.patch text/x-patch 4.3 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Pierre Forstmann 2026-09-05 08:03:45 Re: [PATCH] Remove redundant path_nulls checks in setPathObject/Array
Previous Message Xuneng Zhou 2026-09-05 07:23:18 Re: WAIT FOR NO_THROW option could use some documentation