Content-Type: multipart/alternative;
 boundary="------------sKxs0v3ghZ5i4b1lZ3i0v02Z"
Message-ID: <73eb724a-c1a3-456c-be12-0375d4556b67@gmail.com>
Date: Fri, 28 Aug 2026 15:00:00 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Internal error codes triggered by regression tests and user
 queries, take 2
To: Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Paquier <michael@paquier.xyz>,
 pgsql-hackers <pgsql-hackers@postgresql.org>,
 Justin Pryzby <pryzby@telsasoft.com>
References: <94ac928b-8370-4db5-ada5-579a9ebc089d@gmail.com>
 <697477.1787754418@sss.pgh.pa.us>
 <mosliefjrclrikqpsasjy5ibzowba6vwk4ygimouikiht2i5pb@5g562jqflxsz>
Content-Language: en-US
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: <mosliefjrclrikqpsasjy5ibzowba6vwk4ygimouikiht2i5pb@5g562jqflxsz>
Archived-At: 
 <https://www.postgresql.org/message-id/73eb724a-c1a3-456c-be12-0375d4556b67%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------sKxs0v3ghZ5i4b1lZ3i0v02Z
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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
--------------sKxs0v3ghZ5i4b1lZ3i0v02Z
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 7bit

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <div class="moz-cite-prefix">Hello Andres and Tom,<br>
      <br>
      Thank you for your feedback! <br>
      <br>
      26.08.2026 18:17, Andres Freund wrote:<span
      style="white-space: pre-wrap">
</span></div>
    <blockquote type="cite"
cite="mid:mosliefjrclrikqpsasjy5ibzowba6vwk4ygimouikiht2i5pb@5g562jqflxsz">
      <pre wrap="" class="moz-quote-pre">On 2026-08-26 10:26:58 -0400, Tom Lane wrote:
</pre>
      <blockquote type="cite">
        <pre wrap="" class="moz-quote-pre">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.</pre>
      </blockquote>
    </blockquote>
    <br>
    I agree. The fact that the internal error is reached from a legal
    SQL-level<br>
    function doesn't make all other ways to reach it expected/normal.
    This is<br>
    one issue with this proposal.<br>
    <br>
    The other one is the unclear definition of internal/XX000 errors in<br>
    principle. As far as I can see, it varies from "all the errors that
    have<br>
    no specific code assigned" to "critical errors, similar to failed
    asserts"<br>
    (and probably a substitute for asserts in production). And as Andres
    (and<br>
    I too) find it useful to search for XX000 in production logs, it
    means<br>
    they are interpreted per the second definition. In this case, it
    makes<br>
    sense to define missing codes for cases which are not worth paying<br>
    attention to in production, e.g. "#print_strict_params XXX".
    Moreover, if<br>
    the second definition (XX000 error ~ failed assert) is going to be<br>
    accepted, it seems sensible to have no such errors produced during<br>
    regression tests (like no assertion failures).<br>
    <br>
    I remember previous moves in this direction: [1], [2], so I started
    from<br>
    errors produced just by `make check`. I understand that such cases
    can be<br>
    discussed and processed one-by-one (e.g., bug #19659, #19630,
    #19631,<br>
    #19632), but given the number of "elog(ERROR)" in the tree and the
    number<br>
    of known ways to reach them (dozens, I guess), maybe it would be
    more<br>
    efficient to agree on the definition of internal errors and then<br>
    categorize all the cases at hand (I thought of creating a wiki page
    for<br>
    that).<br>
    <br>
    <blockquote type="cite"
cite="mid:mosliefjrclrikqpsasjy5ibzowba6vwk4ygimouikiht2i5pb@5g562jqflxsz">
      <blockquote type="cite">
        <pre wrap="" class="moz-quote-pre">A lot of these other proposed changes have the same whiff of blaming
the messenger rather than looking for the root cause.
</pre>
      </blockquote>
      <pre wrap="" class="moz-quote-pre">
+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.</pre>
    </blockquote>
    <br>
    Yes, and this consideration (we don't care about error code) is
    closer to<br>
    the definition 1, I guess. I proposed the change to align that error
    with<br>
    the similar "compressed pglz data is corrupt", as it doesn't make
    the<br>
    error less scary if we get back to the "search production logs for<br>
    critical errors" use case (on the other hand, the error is produced
    by a<br>
    test-only function...).<br>
    <br>
    <blockquote type="cite"
cite="mid:mosliefjrclrikqpsasjy5ibzowba6vwk4ygimouikiht2i5pb@5g562jqflxsz">
      <pre wrap="" class="moz-quote-pre">
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.</pre>
    </blockquote>
    <br>
    Yes, it would hardly land as-is, but I hope it can help determine
    ways to<br>
    approach these errors in general.<br>
    <br>
    <blockquote type="cite"
cite="mid:mosliefjrclrikqpsasjy5ibzowba6vwk4ygimouikiht2i5pb@5g562jqflxsz">
      <pre wrap="" class="moz-quote-pre">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).
</pre>
    </blockquote>
    <br>
    I would elaborate on these when the consensus on the definition is
    reached.<br>
    <br>
    [1]
<a class="moz-txt-link-freetext" href="https://www.postgresql.org/message-id/flat/ba76c6bc-f03f-4285-bf16-47759cfcab9e%40eisentraut.org">https://www.postgresql.org/message-id/flat/ba76c6bc-f03f-4285-bf16-47759cfcab9e%40eisentraut.org</a><br>
    [2]
    <a class="moz-txt-link-freetext" href="https://www.postgresql.org/message-id/Zic_GNgos5sMxKoa%40paquier.xyz">https://www.postgresql.org/message-id/Zic_GNgos5sMxKoa%40paquier.xyz</a><br>
    <br>
    Best regards,<br>
    Alexander<br>
  </body>
</html>

--------------sKxs0v3ghZ5i4b1lZ3i0v02Z--