Обсуждение: Remove inconsistent quotes from date_part error

The attached patch corrects a very minor typographical inconsistency
when date_part is invoked with invalid units on time/timetz data vs
timestamp/timestamptz/interval data.

(If stuff like this is too minor to bother with, let me know and I'll
hold off in the future... but since this was pointed out to me I can't
unsee it.)

Nikhil

On Sun, Jan 02, 2022 at 11:47:32PM -0500, Nikhil Benesch wrote:
> The attached patch corrects a very minor typographical inconsistency
> when date_part is invoked with invalid units on time/timetz data vs
> timestamp/timestamptz/interval data.

Hmm, you are right that this is inconsistent, but I don't think that
what you are doing is completely correct either.  First, from what I
can see from the core code, we don't apply quotes to types in error
messages.  So your patch is going in the right direction.

However, there is a specific routine called format_type_be() aimed at
formatting type names for error strings.  If you switch to that, my
guess is that this makes the error messages of time/timetz and
timestamp/timestamptz/interval more consistent, while reducing the
effort of translation because we'd finish with the same base error
string, as of "%s units \"%s\" not recognized".

If we rework this part, we could even rework this error message more.
One suggestion I have would be "units of type %s not recognized", for
example.

> (If stuff like this is too minor to bother with, let me know and I'll
> hold off in the future... but since this was pointed out to me I can't
> unsee it.)

This usually comes down to a case-by-case analysis.  Now, in this
case, your suggestion looks right to me.
--
Michael

On Mon, Jan 3, 2022 at 3:17 AM Michael Paquier <michael@paquier.xyz> wrote:
> However, there is a specific routine called format_type_be() aimed at
> formatting type names for error strings.  If you switch to that, my
> guess is that this makes the error messages of time/timetz and
> timestamp/timestamptz/interval more consistent, while reducing the
> effort of translation because we'd finish with the same base error
> string, as of "%s units \"%s\" not recognized".

I could find only a tiny smattering of examples where format_type_be()
is invoked with a constant OID. In almost all error messages where the
type is statically known, it seems the type name is hardcoded into the
error message rather than generated via format_type_be(). For example,
all of the "TYPE out of range" errors.

I'm happy to rework the patch to use format_type_be(), but wanted to
double check first that it is the preferred approach in this
situation.

Nikhil Benesch <nikhil.benesch@gmail.com> writes:
> I could find only a tiny smattering of examples where format_type_be()
> is invoked with a constant OID. In almost all error messages where the
> type is statically known, it seems the type name is hardcoded into the
> error message rather than generated via format_type_be(). For example,
> all of the "TYPE out of range" errors.

Yeah, but we've been slowly converting to that method to reduce the number
of distinct translatable strings for error messages.  If doing so here
would cut the labor for translators, I'm for it.

            regards, tom lane

On Mon, Jan 3, 2022 at 10:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah, but we've been slowly converting to that method to reduce the number
> of distinct translatable strings for error messages.  If doing so here
> would cut the labor for translators, I'm for it.

Great! I'll update the patch. Thanks for confirming.

Nikhil Benesch <nikhil.benesch@gmail.com> writes:
> -                         errmsg("\"time with time zone\" units \"%s\" not recognized",
> +                         errmsg("time with time zone units \"%s\" not recognized",
> [ etc ]

BTW, if you want to get rid of the quotes, I think that something
else has to be done to set off the type name from the rest.  In
this instance someone might think that we're complaining about a
"time zone unit", whatever that is.  I suggest swapping it around to

    units \"%s\" not recognized for type %s

Also, personally, I'd write unit not units, but that's
more debatable.

            regards, tom lane

On Mon, Jan 3, 2022 at 10:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> BTW, if you want to get rid of the quotes, I think that something
> else has to be done to set off the type name from the rest.  In
> this instance someone might think that we're complaining about a
> "time zone unit", whatever that is.  I suggest swapping it around to
>
>         units \"%s\" not recognized for type %s
>
> Also, personally, I'd write unit not units, but that's
> more debatable.

Your suggestion sounds good to me. I'll update the patch with that.

Not that it changes anything, I think, but the wording ambiguity you
mention is present today in the timestamptz error message:

    benesch=> select extract('nope' from now());
    ERROR:  timestamp with time zone units "nope" not recognized

Updated patch attached.

On Mon, Jan 3, 2022 at 10:26 AM Nikhil Benesch <nikhil.benesch@gmail.com> wrote:
>
> On Mon, Jan 3, 2022 at 10:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > BTW, if you want to get rid of the quotes, I think that something
> > else has to be done to set off the type name from the rest.  In
> > this instance someone might think that we're complaining about a
> > "time zone unit", whatever that is.  I suggest swapping it around to
> >
> >         units \"%s\" not recognized for type %s
> >
> > Also, personally, I'd write unit not units, but that's
> > more debatable.
>
> Your suggestion sounds good to me. I'll update the patch with that.
>
> Not that it changes anything, I think, but the wording ambiguity you
> mention is present today in the timestamptz error message:
>
>     benesch=> select extract('nope' from now());
>     ERROR:  timestamp with time zone units "nope" not recognized

Nikhil Benesch <nikhil.benesch@gmail.com> writes:
> Updated patch attached.

Hmm, I think you went a bit too far here.  The existing code intends
to draw a distinction between "not recognized" (i.e., "we don't know
what that word was you used") and "not supported" (i.e., "we know
that word, but it doesn't seem to make sense in context, or we
haven't got round to the case yet").  You've mashed those into the
same error text, which I don't think we should do, especially
since we're using distinct ERRCODE values for them.

Attached v3 restores that distinction, and makes some other small
tweaks.  (I found that there were actually a couple of spots in
date.c that got it backwards, so admittedly this is a fine point
that not everybody is on board with.  But let's make it consistent
now.)

            regards, tom lane

diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 9e8baeaa9c..629166632d 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -22,6 +22,7 @@
 #include <time.h>

 #include "access/xact.h"
+#include "catalog/pg_type.h"
 #include "common/hashfn.h"
 #include "libpq/pqformat.h"
 #include "miscadmin.h"
@@ -1124,8 +1125,8 @@ extract_date(PG_FUNCTION_ARGS)
             default:
                 ereport(ERROR,
                         (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                         errmsg("date units \"%s\" not supported",
-                                lowunits)));
+                         errmsg("unit \"%s\" not supported for type %s",
+                                lowunits, format_type_be(DATEOID))));
         }
     }
     else if (type == UNITS)
@@ -1207,8 +1208,8 @@ extract_date(PG_FUNCTION_ARGS)
             default:
                 ereport(ERROR,
                         (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                         errmsg("date units \"%s\" not supported",
-                                lowunits)));
+                         errmsg("unit \"%s\" not supported for type %s",
+                                lowunits, format_type_be(DATEOID))));
                 intresult = 0;
         }
     }
@@ -1223,8 +1224,8 @@ extract_date(PG_FUNCTION_ARGS)
             default:
                 ereport(ERROR,
                         (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                         errmsg("date units \"%s\" not supported",
-                                lowunits)));
+                         errmsg("unit \"%s\" not supported for type %s",
+                                lowunits, format_type_be(DATEOID))));
                 intresult = 0;
         }
     }
@@ -1232,7 +1233,8 @@ extract_date(PG_FUNCTION_ARGS)
     {
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                 errmsg("date units \"%s\" not recognized", lowunits)));
+                 errmsg("unit \"%s\" not recognized for type %s",
+                        lowunits, format_type_be(DATEOID))));
         intresult = 0;
     }

@@ -2202,9 +2204,9 @@ time_part_common(PG_FUNCTION_ARGS, bool retnumeric)
             case DTK_ISOYEAR:
             default:
                 ereport(ERROR,
-                        (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                         errmsg("\"time\" units \"%s\" not recognized",
-                                lowunits)));
+                        (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                         errmsg("unit \"%s\" not supported for type %s",
+                                lowunits, format_type_be(TIMEOID))));
                 intresult = 0;
         }
     }
@@ -2219,8 +2221,8 @@ time_part_common(PG_FUNCTION_ARGS, bool retnumeric)
     {
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                 errmsg("\"time\" units \"%s\" not recognized",
-                        lowunits)));
+                 errmsg("unit \"%s\" not recognized for type %s",
+                        lowunits, format_type_be(TIMEOID))));
         intresult = 0;
     }

@@ -2980,9 +2982,9 @@ timetz_part_common(PG_FUNCTION_ARGS, bool retnumeric)
             case DTK_MILLENNIUM:
             default:
                 ereport(ERROR,
-                        (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                         errmsg("\"time with time zone\" units \"%s\" not recognized",
-                                lowunits)));
+                        (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                         errmsg("unit \"%s\" not supported for type %s",
+                                lowunits, format_type_be(TIMETZOID))));
                 intresult = 0;
         }
     }
@@ -3001,8 +3003,8 @@ timetz_part_common(PG_FUNCTION_ARGS, bool retnumeric)
     {
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                 errmsg("\"time with time zone\" units \"%s\" not recognized",
-                        lowunits)));
+                 errmsg("unit \"%s\" not recognized for type %s",
+                        lowunits, format_type_be(TIMETZOID))));
         intresult = 0;
     }

diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index cb9faff0bb..b66d69db00 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -3972,8 +3972,8 @@ timestamp_trunc(PG_FUNCTION_ARGS)
             default:
                 ereport(ERROR,
                         (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                         errmsg("timestamp units \"%s\" not supported",
-                                lowunits)));
+                         errmsg("unit \"%s\" not supported for type %s",
+                                lowunits, format_type_be(TIMESTAMPOID))));
                 result = 0;
         }

@@ -3986,8 +3986,8 @@ timestamp_trunc(PG_FUNCTION_ARGS)
     {
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                 errmsg("timestamp units \"%s\" not recognized",
-                        lowunits)));
+                 errmsg("unit \"%s\" not recognized for type %s",
+                        lowunits, format_type_be(TIMESTAMPOID))));
         result = 0;
     }

@@ -4165,8 +4165,8 @@ timestamptz_trunc_internal(text *units, TimestampTz timestamp, pg_tz *tzp)
             default:
                 ereport(ERROR,
                         (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                         errmsg("timestamp with time zone units \"%s\" not "
-                                "supported", lowunits)));
+                         errmsg("unit \"%s\" not supported for type %s",
+                                lowunits, format_type_be(TIMESTAMPTZOID))));
                 result = 0;
         }

@@ -4182,8 +4182,8 @@ timestamptz_trunc_internal(text *units, TimestampTz timestamp, pg_tz *tzp)
     {
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                 errmsg("timestamp with time zone units \"%s\" not recognized",
-                        lowunits)));
+                 errmsg("unit \"%s\" not recognized for type %s",
+                        lowunits, format_type_be(TIMESTAMPTZOID))));
         result = 0;
     }

@@ -4337,17 +4337,11 @@ interval_trunc(PG_FUNCTION_ARGS)
                     break;

                 default:
-                    if (val == DTK_WEEK)
-                        ereport(ERROR,
-                                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                                 errmsg("interval units \"%s\" not supported "
-                                        "because months usually have fractional weeks",
-                                        lowunits)));
-                    else
-                        ereport(ERROR,
-                                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                                 errmsg("interval units \"%s\" not supported",
-                                        lowunits)));
+                    ereport(ERROR,
+                            (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                             errmsg("unit \"%s\" not supported for type %s",
+                                    lowunits, format_type_be(INTERVALOID)),
+                             (val == DTK_WEEK) ? errdetail("Months usually have fractional weeks.") : 0));
             }

             if (tm2interval(tm, fsec, result) != 0)
@@ -4362,8 +4356,8 @@ interval_trunc(PG_FUNCTION_ARGS)
     {
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                 errmsg("interval units \"%s\" not recognized",
-                        lowunits)));
+                 errmsg("unit \"%s\" not recognized for type %s",
+                        lowunits, format_type_be(INTERVALOID))));
     }

     PG_RETURN_INTERVAL_P(result);
@@ -4559,18 +4553,11 @@ NonFiniteTimestampTzPart(int type, int unit, char *lowunits,
                          bool isNegative, bool isTz)
 {
     if ((type != UNITS) && (type != RESERV))
-    {
-        if (isTz)
-            ereport(ERROR,
-                    (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                     errmsg("timestamp with time zone units \"%s\" not recognized",
-                            lowunits)));
-        else
-            ereport(ERROR,
-                    (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                     errmsg("timestamp units \"%s\" not recognized",
-                            lowunits)));
-    }
+        ereport(ERROR,
+                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                 errmsg("unit \"%s\" not recognized for type %s",
+                        lowunits,
+                        format_type_be(isTz ? TIMESTAMPTZOID : TIMESTAMPOID))));

     switch (unit)
     {
@@ -4606,16 +4593,11 @@ NonFiniteTimestampTzPart(int type, int unit, char *lowunits,
                 return get_float8_infinity();

         default:
-            if (isTz)
-                ereport(ERROR,
-                        (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                         errmsg("timestamp with time zone units \"%s\" not supported",
-                                lowunits)));
-            else
-                ereport(ERROR,
-                        (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                         errmsg("timestamp units \"%s\" not supported",
-                                lowunits)));
+            ereport(ERROR,
+                    (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                     errmsg("unit \"%s\" not supported for type %s",
+                            lowunits,
+                            format_type_be(isTz ? TIMESTAMPTZOID : TIMESTAMPOID))));
             return 0.0;            /* keep compiler quiet */
     }
 }
@@ -4814,8 +4796,8 @@ timestamp_part_common(PG_FUNCTION_ARGS, bool retnumeric)
             default:
                 ereport(ERROR,
                         (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                         errmsg("timestamp units \"%s\" not supported",
-                                lowunits)));
+                         errmsg("unit \"%s\" not supported for type %s",
+                                lowunits, format_type_be(TIMESTAMPOID))));
                 intresult = 0;
         }
     }
@@ -4861,8 +4843,8 @@ timestamp_part_common(PG_FUNCTION_ARGS, bool retnumeric)
             default:
                 ereport(ERROR,
                         (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                         errmsg("timestamp units \"%s\" not supported",
-                                lowunits)));
+                         errmsg("unit \"%s\" not supported for type %s",
+                                lowunits, format_type_be(TIMESTAMPOID))));
                 intresult = 0;
         }

@@ -4871,7 +4853,8 @@ timestamp_part_common(PG_FUNCTION_ARGS, bool retnumeric)
     {
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                 errmsg("timestamp units \"%s\" not recognized", lowunits)));
+                 errmsg("unit \"%s\" not recognized for type %s",
+                        lowunits, format_type_be(TIMESTAMPOID))));
         intresult = 0;
     }

@@ -5085,8 +5068,8 @@ timestamptz_part_common(PG_FUNCTION_ARGS, bool retnumeric)
             default:
                 ereport(ERROR,
                         (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                         errmsg("timestamp with time zone units \"%s\" not supported",
-                                lowunits)));
+                         errmsg("unit \"%s\" not supported for type %s",
+                                lowunits, format_type_be(TIMESTAMPTZOID))));
                 intresult = 0;
         }

@@ -5133,8 +5116,8 @@ timestamptz_part_common(PG_FUNCTION_ARGS, bool retnumeric)
             default:
                 ereport(ERROR,
                         (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                         errmsg("timestamp with time zone units \"%s\" not supported",
-                                lowunits)));
+                         errmsg("unit \"%s\" not supported for type %s",
+                                lowunits, format_type_be(TIMESTAMPTZOID))));
                 intresult = 0;
         }
     }
@@ -5142,8 +5125,8 @@ timestamptz_part_common(PG_FUNCTION_ARGS, bool retnumeric)
     {
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                 errmsg("timestamp with time zone units \"%s\" not recognized",
-                        lowunits)));
+                 errmsg("unit \"%s\" not recognized for type %s",
+                        lowunits, format_type_be(TIMESTAMPTZOID))));

         intresult = 0;
     }
@@ -5265,8 +5248,8 @@ interval_part_common(PG_FUNCTION_ARGS, bool retnumeric)
                 default:
                     ereport(ERROR,
                             (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                             errmsg("interval units \"%s\" not supported",
-                                    lowunits)));
+                             errmsg("unit \"%s\" not supported for type %s",
+                                    lowunits, format_type_be(INTERVALOID))));
                     intresult = 0;
             }
         }
@@ -5326,8 +5309,8 @@ interval_part_common(PG_FUNCTION_ARGS, bool retnumeric)
     {
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                 errmsg("interval units \"%s\" not recognized",
-                        lowunits)));
+                 errmsg("unit \"%s\" not recognized for type %s",
+                        lowunits, format_type_be(INTERVALOID))));
         intresult = 0;
     }

diff --git a/src/test/regress/expected/date.out b/src/test/regress/expected/date.out
index c8b0566ff4..75ff659378 100644
--- a/src/test/regress/expected/date.out
+++ b/src/test/regress/expected/date.out
@@ -1129,15 +1129,15 @@ SELECT EXTRACT(DECADE FROM DATE '0012-12-31 BC'); --  -2
 -- all possible fields
 --
 SELECT EXTRACT(MICROSECONDS  FROM DATE '2020-08-11');
-ERROR:  date units "microseconds" not supported
+ERROR:  unit "microseconds" not supported for type date
 SELECT EXTRACT(MILLISECONDS  FROM DATE '2020-08-11');
-ERROR:  date units "milliseconds" not supported
+ERROR:  unit "milliseconds" not supported for type date
 SELECT EXTRACT(SECOND        FROM DATE '2020-08-11');
-ERROR:  date units "second" not supported
+ERROR:  unit "second" not supported for type date
 SELECT EXTRACT(MINUTE        FROM DATE '2020-08-11');
-ERROR:  date units "minute" not supported
+ERROR:  unit "minute" not supported for type date
 SELECT EXTRACT(HOUR          FROM DATE '2020-08-11');
-ERROR:  date units "hour" not supported
+ERROR:  unit "hour" not supported for type date
 SELECT EXTRACT(DAY           FROM DATE '2020-08-11');
  extract
 ---------
@@ -1235,11 +1235,11 @@ SELECT EXTRACT(DOY           FROM DATE '2020-08-11');
 (1 row)

 SELECT EXTRACT(TIMEZONE      FROM DATE '2020-08-11');
-ERROR:  date units "timezone" not supported
+ERROR:  unit "timezone" not supported for type date
 SELECT EXTRACT(TIMEZONE_M    FROM DATE '2020-08-11');
-ERROR:  date units "timezone_m" not supported
+ERROR:  unit "timezone_m" not supported for type date
 SELECT EXTRACT(TIMEZONE_H    FROM DATE '2020-08-11');
-ERROR:  date units "timezone_h" not supported
+ERROR:  unit "timezone_h" not supported for type date
 SELECT EXTRACT(EPOCH         FROM DATE '2020-08-11');
   extract
 ------------
@@ -1462,7 +1462,7 @@ SELECT EXTRACT(EPOCH      FROM DATE 'infinity');    --  Infinity
 -- wrong fields from non-finite date:
 --
 SELECT EXTRACT(MICROSEC  FROM DATE 'infinity');     -- error
-ERROR:  date units "microsec" not recognized
+ERROR:  unit "microsec" not recognized for type date
 -- test constructors
 select make_date(2013, 7, 15);
  make_date
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index e4b1246f45..accd4a7d90 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -963,9 +963,9 @@ SELECT f1,
 (10 rows)

 SELECT EXTRACT(FORTNIGHT FROM INTERVAL '2 days');  -- error
-ERROR:  interval units "fortnight" not recognized
+ERROR:  unit "fortnight" not recognized for type interval
 SELECT EXTRACT(TIMEZONE FROM INTERVAL '2 days');  -- error
-ERROR:  interval units "timezone" not supported
+ERROR:  unit "timezone" not supported for type interval
 SELECT EXTRACT(DECADE FROM INTERVAL '100 y');
  extract
 ---------
diff --git a/src/test/regress/expected/time.out b/src/test/regress/expected/time.out
index 39b409feca..f3a71503c2 100644
--- a/src/test/regress/expected/time.out
+++ b/src/test/regress/expected/time.out
@@ -161,11 +161,11 @@ SELECT EXTRACT(HOUR        FROM TIME '2020-05-26 13:30:25.575401');
 (1 row)

 SELECT EXTRACT(DAY         FROM TIME '2020-05-26 13:30:25.575401');  -- error
-ERROR:  "time" units "day" not recognized
+ERROR:  unit "day" not supported for type time without time zone
 SELECT EXTRACT(FORTNIGHT   FROM TIME '2020-05-26 13:30:25.575401');  -- error
-ERROR:  "time" units "fortnight" not recognized
+ERROR:  unit "fortnight" not recognized for type time without time zone
 SELECT EXTRACT(TIMEZONE    FROM TIME '2020-05-26 13:30:25.575401');  -- error
-ERROR:  "time" units "timezone" not recognized
+ERROR:  unit "timezone" not supported for type time without time zone
 SELECT EXTRACT(EPOCH       FROM TIME '2020-05-26 13:30:25.575401');
    extract
 --------------
diff --git a/src/test/regress/expected/timetz.out b/src/test/regress/expected/timetz.out
index f4960c0166..8942a9b95b 100644
--- a/src/test/regress/expected/timetz.out
+++ b/src/test/regress/expected/timetz.out
@@ -178,9 +178,9 @@ SELECT EXTRACT(HOUR        FROM TIME WITH TIME ZONE '2020-05-26 13:30:25.575401-
 (1 row)

 SELECT EXTRACT(DAY         FROM TIME WITH TIME ZONE '2020-05-26 13:30:25.575401-04');  -- error
-ERROR:  "time with time zone" units "day" not recognized
+ERROR:  unit "day" not supported for type time with time zone
 SELECT EXTRACT(FORTNIGHT   FROM TIME WITH TIME ZONE '2020-05-26 13:30:25.575401-04');  -- error
-ERROR:  "time with time zone" units "fortnight" not recognized
+ERROR:  unit "fortnight" not recognized for type time with time zone
 SELECT EXTRACT(TIMEZONE    FROM TIME WITH TIME ZONE '2020-05-26 13:30:25.575401-04:30');
  extract
 ---------

On 2022-Jan-03, Tom Lane wrote:

> Attached v3 restores that distinction, and makes some other small
> tweaks.  (I found that there were actually a couple of spots in
> date.c that got it backwards, so admittedly this is a fine point
> that not everybody is on board with.  But let's make it consistent
> now.)

LGTM.

> @@ -2202,9 +2204,9 @@ time_part_common(PG_FUNCTION_ARGS, bool retnumeric)
>              case DTK_ISOYEAR:
>              default:
>                  ereport(ERROR,
> -                        (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> -                         errmsg("\"time\" units \"%s\" not recognized",
> -                                lowunits)));
> +                        (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +                         errmsg("unit \"%s\" not supported for type %s",
> +                                lowunits, format_type_be(TIMEOID))));

I agree that these changes are an improvement.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Mon, Jan 3, 2022 at 1:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Hmm, I think you went a bit too far here.  The existing code intends
> to draw a distinction between "not recognized" (i.e., "we don't know
> what that word was you used") and "not supported" (i.e., "we know
> that word, but it doesn't seem to make sense in context, or we
> haven't got round to the case yet").  You've mashed those into the
> same error text, which I don't think we should do, especially
> since we're using distinct ERRCODE values for them.

Oops. I noticed that "inconsistency" between
ERRCODE_FEATURE_NOT_SUPPORTED and ERRCODE_INVALID_PARAMETER_VALUE and
then promptly blazed past it. Thanks for catching that.

> Attached v3 restores that distinction, and makes some other small
> tweaks.  (I found that there were actually a couple of spots in
> date.c that got it backwards, so admittedly this is a fine point
> that not everybody is on board with.  But let's make it consistent
> now.)

LGTM too, for whatever that's worth.

Nikhil Benesch <nikhil.benesch@gmail.com> writes:
> On Mon, Jan 3, 2022 at 1:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Attached v3 restores that distinction, and makes some other small
>> tweaks.  (I found that there were actually a couple of spots in
>> date.c that got it backwards, so admittedly this is a fine point
>> that not everybody is on board with.  But let's make it consistent
>> now.)

> LGTM too, for whatever that's worth.

OK, pushed.

            regards, tom lane