Content-Type: multipart/alternative;
 boundary="------------403bC9dRxpSzyQlyo3rQo8yA"
Message-ID: <8343126e-11e5-48df-9f96-ea03a2b7d5ba@gmail.com>
Date: Mon, 7 Sep 2026 00: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: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Paquier <michael@paquier.xyz>, Andres Freund
 <andres@anarazel.de>, 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>
 <73eb724a-c1a3-456c-be12-0375d4556b67@gmail.com>
 <apUcBVyazHEW5pcg@paquier.xyz>
 <91a1b0ab-2c6c-4471-b7a4-ee8ee8aed64e@gmail.com>
 <126103.1788721678@sss.pgh.pa.us>
Content-Language: en-US
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: <126103.1788721678@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/8343126e-11e5-48df-9f96-ea03a2b7d5ba%40gmail.com>
Precedence: bulk

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

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/20230213135053.GZ1653@telsasoft.com

Best regards,
Alexander
--------------403bC9dRxpSzyQlyo3rQo8yA
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <div class="moz-cite-prefix">Hello Tom,<br>
      <br>
      06.09.2026 22:07, Tom Lane wrote:<br>
    </div>
    <blockquote type="cite" cite="mid:126103.1788721678@sss.pgh.pa.us">
      <pre wrap="" class="moz-quote-pre">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-&gt;classId);
+            ereport(ERROR,
+                    (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                     errmsg("unsupported object class: %u", object-&gt;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.</pre>
    </blockquote>
    <br>
    Thank you for the clarification! Will change this way.<br>
    <br>
    <blockquote type="cite" cite="mid:126103.1788721678@sss.pgh.pa.us">
      <pre wrap="" class="moz-quote-pre">@@ -794,8 +794,10 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
                     if (constraint-&gt;conname &amp;&amp;
                         notnull_constraint-&gt;conname &amp;&amp;
                         strcmp(notnull_constraint-&gt;conname, constraint-&gt;conname) != 0)
-                        elog(ERROR, "conflicting not-null constraint names \"%s\" and \"%s\"",
-                             notnull_constraint-&gt;conname, constraint-&gt;conname);
+                        ereport(ERROR,
+                                errcode(ERRCODE_SYNTAX_ERROR),
+                                errmsg("conflicting not-null constraint names \"%s\" and \"%s\"",
+                                       notnull_constraint-&gt;conname, constraint-&gt;conname));

This does not seem like a "syntax error".  Perhaps
ERRCODE_INVALID_OBJECT_DEFINITION would serve?</pre>
    </blockquote>
    <br>
    Yeah, I considered this, but the same errmsg in<br>
    AddRelationNotNullConstraints() uses ERRCODE_SYNTAX_ERROR. I found
    it<br>
    more consistent to emit the same code. Maybe it would make do the
    opposite<br>
    -- change the errcode in AddRelationNotNullConstraints() (there are
    two<br>
    instances there, though)... Moreover, ERRCODE_SYNTAX_ERROR is chosen
    for<br>
    many other similar messages in transformColumnDefinition(), so I'm
    not<br>
    sure if it makes sense to change all of those or just report "syntax
    error"<br>
    for consistency while keeping the patch focused.<br>
    <br>
    <blockquote type="cite" cite="mid:126103.1788721678@sss.pgh.pa.us">
      <pre wrap="" class="moz-quote-pre">@@ -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).</pre>
    </blockquote>
    <br>
    Thank you for the suggestion! Will change the code and add the
    protection.<br>
    <br>
    <blockquote type="cite" cite="mid:126103.1788721678@sss.pgh.pa.us">
      <pre wrap="" class="moz-quote-pre">@@ -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</pre>
    </blockquote>
    <br>
    Yeah, I have this one in my collection too, but started from
    internal<br>
    errors reported previously -- that one at [1], it is reachable with,<br>
    e.g.:<br>
    SELECT pg_catalog.interval('1 second', 0);<br>
    ERROR:  XX000: unrecognized interval typmod: 0<br>
    LOCATION:  AdjustIntervalForTypmod, timestamp.c:1492<br>
    <br>
    <blockquote type="cite" cite="mid:126103.1788721678@sss.pgh.pa.us">
      <pre wrap="" class="moz-quote-pre">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.</pre>
    </blockquote>
    <br>
    Will check them and include into this patch.<br>
    <br>
    <blockquote type="cite" cite="mid:126103.1788721678@sss.pgh.pa.us">
      <pre wrap="" class="moz-quote-pre">@@ -393,7 +393,9 @@ comp_option        : '#' K_OPTION K_DUMP
                         else if (strcmp($3, "off") == 0)
                             plpgsql_curr_compile-&gt;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.</pre>
    </blockquote>
    <span style="white-space: pre-wrap">
</span><br>
    Yes, I'm not aware of cases where one errmsg is issued with
    different<br>
    codes (probably there may be such cases with very generic messages),
    but<br>
    I'd stick to reusing existing code if it's not too wrong.<br>
    <br>
    [1]
<a class="moz-txt-link-freetext" href="https://www.postgresql.org/message-id/20230213135053.GZ1653@telsasoft.com">https://www.postgresql.org/message-id/20230213135053.GZ1653@telsasoft.com</a><br>
    <br>
    Best regards,<br>
    Alexander<br>
  </body>
</html>

--------------403bC9dRxpSzyQlyo3rQo8yA--