Remove inconsistent quotes from date_part error

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.

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

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