Hello Andres and Tom,
Thank you for your feedback!
26.08.2026 18:17, Andres Freund wrote:
On 2026-08-26 10:26:58 -0400, Tom Lane wrote:
I'm not on board with these proposed changes to [try_]relation_open.
They're basically band-aids rather than fixes of the root problems.
That is, if someone passes OID 0 to relation_open, that IS an internal
bug; labeling it otherwise is a lie and does nothing to fix the real
problem at the caller level.
I agree. The fact that the internal error is reached from a legal SQL-level
function doesn't make all other ways to reach it expected/normal. This is
one issue with this proposal.
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 remember previous moves in this direction: [1], [2], so I started from
errors produced just by `make check`. I understand that such cases can be
discussed and processed one-by-one (e.g., bug #19659, #19630, #19631,
#19632), but given the number of "elog(ERROR)" in the tree and the number
of known ways to reach them (dozens, I guess), maybe it would be more
efficient to agree on the definition of internal errors and then
categorize all the cases at hand (I thought of creating a wiki page for
that).
A lot of these other proposed changes have the same whiff of blaming
the messenger rather than looking for the root cause.
+1.
This is triply true for stuff like test_pglz_decompress() - this isn't
something that should be translated or that we care about having a proper
error code assigned.
Yes, and this consideration (we don't care about error code) is closer to
the definition 1, I guess. I proposed the change to align that error with
the similar "compressed pglz data is corrupt", as it doesn't make the
error less scary if we get back to the "search production logs for
critical errors" use case (on the other hand, the error is produced by a
test-only function...).
Most of these would make it *harder* to find unexpected scenarios, i.e. the
patch would make the situation strictly worse. It's useful to be able to
search production logs for internal errors. As-is the patch would break that.
Yes, it would hardly land as-is, but I hope it can help determine ways to
approach these errors in general.
Looking through them, the ones that look relatively clearly to be worth
turning into ereports seem to be:
- pg_get_shmem_allocations_numa() - there's really no reason for this to be an
elog(), it's obviously expected to be reached on a bunch of platforms /
configurations
- transformColumnDefinition() - that's clearly reachable with plain DDL,
without representing a bug in an upper layer.
I'm a bit on the fence about the one get_range_io_data(). We could catch that
on a higher layer, but
a) that'd be duplicated code,
b) other errors, like not having send/receive are already handled in
get_range_io_data, with ereports()
c) for e.g. composites, we handle the same problem centrally, rather than have
checks in record_in() etc (the check is in lookup_rowtype_tupdesc_internal).
I would elaborate on these when the consensus on the definition is reached.
[1]
https://www.postgresql.org/message-id/flat/ba76c6bc-f03f-4285-bf16-47759cfcab9e%40eisentraut.org
[2] https://www.postgresql.org/message-id/Zic_GNgos5sMxKoa%40paquier.xyz
Best regards,
Alexander
