Remove an unnecessary errmsg_plural in dependency.c

Started by Bharath Rupireddyalmost 4 years ago15 messages
#1Bharath Rupireddy
bharath.rupireddyforpostgres@gmail.com
1 attachment(s)

Hi,

It looks like the following errmsg_plural in dependency.c is
unnecessary as numReportedClient > 1 always and numNotReportedClient
can never be < 0. Therefore plural version of the error message is
sufficient. Attached a patch to fix it.

@@ -1200,10 +1200,8 @@ reportDependentObjects(const ObjectAddresses
*targetObjects,
        {
                ereport(msglevel,
                /* translator: %d always has a value larger than 1 */
-                               (errmsg_plural("drop cascades to %d
other object",
-                                                          "drop
cascades to %d other objects",
-
numReportedClient + numNotReportedClient,
-
numReportedClient + numNotReportedClient),
+                               (errmsg("drop cascades to %d other objects",
+                                                numReportedClient +
numNotReportedClient),
                                 errdetail("%s", clientdetail.data),
                                 errdetail_log("%s", logdetail.data)));

Regards,
Bharath Rupireddy.

Attachments:

v1-0001-Remove-an-unnecessary-errmsg_plural-in-dependency.patchapplication/x-patch; name=v1-0001-Remove-an-unnecessary-errmsg_plural-in-dependency.patchDownload
From 50328d92744f279bf26e72b6dc18c4dd9679e4aa Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Wed, 23 Mar 2022 16:20:56 +0000
Subject: [PATCH v1] Remove an unnecessary errmsg_plural in dependency.c
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The errmsg_plural for "drop cascades to %d other objects" is
unnecessary as numReportedClient > 1 always and
numNotReportedClient can never be < 0. Therefore plural version of
the error message is sufficient.
---
 src/backend/catalog/dependency.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index ab9e42d7d1..2ca4492620 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -1200,10 +1200,8 @@ reportDependentObjects(const ObjectAddresses *targetObjects,
 	{
 		ereport(msglevel,
 		/* translator: %d always has a value larger than 1 */
-				(errmsg_plural("drop cascades to %d other object",
-							   "drop cascades to %d other objects",
-							   numReportedClient + numNotReportedClient,
-							   numReportedClient + numNotReportedClient),
+				(errmsg("drop cascades to %d other objects",
+						 numReportedClient + numNotReportedClient),
 				 errdetail("%s", clientdetail.data),
 				 errdetail_log("%s", logdetail.data)));
 	}
-- 
2.25.1

#2Peter Eisentraut
peter.eisentraut@enterprisedb.com
In reply to: Bharath Rupireddy (#1)
Re: Remove an unnecessary errmsg_plural in dependency.c

On 23.03.22 17:33, Bharath Rupireddy wrote:

It looks like the following errmsg_plural in dependency.c is
unnecessary as numReportedClient > 1 always and numNotReportedClient
can never be < 0. Therefore plural version of the error message is
sufficient. Attached a patch to fix it.

Some languages have more than two forms, so we still need to keep this
to handle those.

#3Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: Peter Eisentraut (#2)
Re: Remove an unnecessary errmsg_plural in dependency.c

At Wed, 23 Mar 2022 17:39:52 +0100, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote in

On 23.03.22 17:33, Bharath Rupireddy wrote:

It looks like the following errmsg_plural in dependency.c is
unnecessary as numReportedClient > 1 always and numNotReportedClient
can never be < 0. Therefore plural version of the error message is
sufficient. Attached a patch to fix it.

Some languages have more than two forms, so we still need to keep this
to handle those.

The point seems to be that numReportedClient + numNotReportedClient >=
2 (not 1) there. So the singular form is never used. It doesn't harm
as-is but translation burden decreases a bit by fixing it.

By the way it has a translator-note as follows.

else if (numReportedClient > 1)
{
ereport(msglevel,
/* translator: %d always has a value larger than 1 */
(errmsg_plural("drop cascades to %d other object",
"drop cascades to %d other objects",
numReportedClient + numNotReportedClient,
numReportedClient + numNotReportedClient),

The comment and errmsg_plural don't seem to be consistent. When the
code was added by c4f2a0458d, it had only singular form and already
had the comment. After that 8032d76b5 turned it to errmsg_plural
ignoring the comment. It seems like a thinko of 8032d76b5.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#4Daniel Gustafsson
daniel@yesql.se
In reply to: Kyotaro Horiguchi (#3)
Re: Remove an unnecessary errmsg_plural in dependency.c

On 24 Mar 2022, at 06:17, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

The comment and errmsg_plural don't seem to be consistent. When the
code was added by c4f2a0458d, it had only singular form and already
had the comment. After that 8032d76b5 turned it to errmsg_plural
ignoring the comment. It seems like a thinko of 8032d76b5.

Following the bouncing ball, that seems like a reasonable conclusion, and
removing the plural form should be fine to reduce translator work.

--
Daniel Gustafsson https://vmware.com/

#5Bharath Rupireddy
bharath.rupireddyforpostgres@gmail.com
In reply to: Daniel Gustafsson (#4)
Re: Remove an unnecessary errmsg_plural in dependency.c

On Thu, Mar 24, 2022 at 2:34 PM Daniel Gustafsson <daniel@yesql.se> wrote:

On 24 Mar 2022, at 06:17, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

The comment and errmsg_plural don't seem to be consistent. When the
code was added by c4f2a0458d, it had only singular form and already
had the comment. After that 8032d76b5 turned it to errmsg_plural
ignoring the comment. It seems like a thinko of 8032d76b5.

Following the bouncing ball, that seems like a reasonable conclusion, and
removing the plural form should be fine to reduce translator work.

Yes, the singular version of the message isn't required at all as
numReportedClient > 1. Hence I proposed to remove errmsg_plural and
singular version.

Regards,
Bharath Rupireddy.

#6Peter Eisentraut
peter.eisentraut@enterprisedb.com
In reply to: Bharath Rupireddy (#5)
Re: Remove an unnecessary errmsg_plural in dependency.c

On 24.03.22 13:48, Bharath Rupireddy wrote:

Yes, the singular version of the message isn't required at all as
numReportedClient > 1. Hence I proposed to remove errmsg_plural and
singular version.

The issue is that n == 1 and n != 1 are not the only cases that
errmsg_plural() handles. Some languages have different forms for n ==
1, n == 2, and n >= 5, for example. So while it is true that in

errmsg_plural("drop cascades to %d other object",
"drop cascades to %d other objects",

the English singular string will never be used, you have to keep the
errmsg_plural() call so that it can handle variants like the above for
other languages.

You could write

errmsg_plural("DUMMY NOT USED %d",
"drop cascades to %d other objects",

but I don't think that is better.

#7Peter Eisentraut
peter.eisentraut@enterprisedb.com
In reply to: Kyotaro Horiguchi (#3)
Re: Remove an unnecessary errmsg_plural in dependency.c

On 24.03.22 06:17, Kyotaro Horiguchi wrote:

The comment and errmsg_plural don't seem to be consistent. When the
code was added by c4f2a0458d, it had only singular form and already
had the comment. After that 8032d76b5 turned it to errmsg_plural
ignoring the comment. It seems like a thinko of 8032d76b5.

I have removed the comment.

#8Daniel Gustafsson
daniel@yesql.se
In reply to: Peter Eisentraut (#7)
Re: Remove an unnecessary errmsg_plural in dependency.c

On 24 Mar 2022, at 14:07, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:

On 24.03.22 06:17, Kyotaro Horiguchi wrote:

The comment and errmsg_plural don't seem to be consistent. When the
code was added by c4f2a0458d, it had only singular form and already
had the comment. After that 8032d76b5 turned it to errmsg_plural
ignoring the comment. It seems like a thinko of 8032d76b5.

I have removed the comment.

I was just typing a reply to your upthread answer that we should just remove
the comment then, so a retroactive +1 on this =)

--
Daniel Gustafsson https://vmware.com/

#9Bharath Rupireddy
bharath.rupireddyforpostgres@gmail.com
In reply to: Peter Eisentraut (#6)
Re: Remove an unnecessary errmsg_plural in dependency.c

On Thu, Mar 24, 2022 at 6:35 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 24.03.22 13:48, Bharath Rupireddy wrote:

Yes, the singular version of the message isn't required at all as
numReportedClient > 1. Hence I proposed to remove errmsg_plural and
singular version.

The issue is that n == 1 and n != 1 are not the only cases that
errmsg_plural() handles. Some languages have different forms for n ==
1, n == 2, and n >= 5, for example. So while it is true that in

errmsg_plural("drop cascades to %d other object",
"drop cascades to %d other objects",

Thanks. I think I get the point - is it dngettext doing things
differently for different languages?

#define EVALUATE_MESSAGE_PLURAL(domain, targetfield, appendval) \
{ \
const char *fmt; \
StringInfoData buf; \
/* Internationalize the error format string */ \
if (!in_error_recursion_trouble()) \
fmt = dngettext((domain), fmt_singular, fmt_plural, n); \
else \
fmt = (n == 1 ? fmt_singular : fmt_plural); \
initStringInfo(&buf); \

Regards,
Bharath Rupireddy.

#10Tom Lane
tgl@sss.pgh.pa.us
In reply to: Bharath Rupireddy (#9)
Re: Remove an unnecessary errmsg_plural in dependency.c

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:

Thanks. I think I get the point - is it dngettext doing things
differently for different languages?

Yeah. To be concrete, have a look in ru.po:

#: catalog/dependency.c:1208
#, c-format
msgid "drop cascades to %d other object"
msgid_plural "drop cascades to %d other objects"
msgstr[0] "удаление распространяется на ещё %d объект"
msgstr[1] "удаление распространяется на ещё %d объекта"
msgstr[2] "удаление распространяется на ещё %d объектов"

I don't know Russian, so I don't know exactly what's going
on there, but there's evidently three different forms in
that language. Probably one of them is not reachable given
that n > 1, but I doubt we're saving the translator any time
with that info. Besides, gettext might require all three
forms to be provided anyway in order to work correctly.

regards, tom lane

#11Alvaro Herrera
alvherre@alvh.no-ip.org
In reply to: Bharath Rupireddy (#9)
Re: Remove an unnecessary errmsg_plural in dependency.c

On 2022-Mar-24, Bharath Rupireddy wrote:

Thanks. I think I get the point - is it dngettext doing things
differently for different languages?

Yes. The dngettext() rules are embedded in each translation's catalog
file:

$ git grep 'Plural-Forms' src/backend/po/*.po
de.po:"Plural-Forms: nplurals=2; plural=(n != 1);\n"
es.po:"Plural-Forms: nplurals=2; plural=n != 1;\n"
fr.po:"Plural-Forms: nplurals=2; plural=(n > 1);\n"
id.po:"Plural-Forms: nplurals=2; plural=(n > 1);\n"
it.po:"Plural-Forms: nplurals=2; plural=n != 1;\n"
ja.po:"Plural-Forms: nplurals=2; plural=n != 1;\n"
ko.po:"Plural-Forms: nplurals=1; plural=0;\n"
pl.po:"Plural-Forms: nplurals=3; plural=(n==1 ? 0 : n%10>=2 && n%10<=4 && (n%100<10 "
pt_BR.po:"Plural-Forms: nplurals=2; plural=(n>1);\n"
ru.po:"Plural-Forms: nplurals=3; plural=(n%10==1 && n%100!=11 ? 0 : n%10>=2 && n"
sv.po:"Plural-Forms: nplurals=2; plural=(n != 1);\n"
tr.po:"Plural-Forms: nplurals=2; plural=(n != 1);\n"
uk.po:"Plural-Forms: nplurals=4; plural=((n%10==1 && n%100!=11) ? 0 : ((n%10 >= 2 && n%10 <=4 && (n%100 < 12 || n%100 > 14)) ? 1 : ((n%10 == 0 || (n%10 >= 5 && n%10 <=9)) || (n%100 >= 11 && n%100 <= 14)) ? 2 : 3));\n"
zh_CN.po:"Plural-Forms: nplurals=1; plural=0;\n"

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Thou shalt study thy libraries and strive not to reinvent them without
cause, that thy code may be short and readable and thy days pleasant
and productive. (7th Commandment for C Programmers)

#12Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#11)
Re: Remove an unnecessary errmsg_plural in dependency.c

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

$ git grep 'Plural-Forms' src/backend/po/*.po
ru.po:"Plural-Forms: nplurals=3; plural=(n%10==1 && n%100!=11 ? 0 : n%10>=2 && n"

Oh, interesting: if I'm reading that right, all three Russian
forms are reachable, even with the knowledge that n > 1.
(But isn't the last "&& n" test redundant?)

regards, tom lane

#13Alvaro Herrera
alvherre@alvh.no-ip.org
In reply to: Tom Lane (#12)
Re: Remove an unnecessary errmsg_plural in dependency.c

On 2022-Mar-24, Tom Lane wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

$ git grep 'Plural-Forms' src/backend/po/*.po
ru.po:"Plural-Forms: nplurals=3; plural=(n%10==1 && n%100!=11 ? 0 : n%10>=2 && n"

Oh, interesting: if I'm reading that right, all three Russian
forms are reachable, even with the knowledge that n > 1.
(But isn't the last "&& n" test redundant?)

I wondered about that trailing 'n' and it turns out that the grep was
too simplistic, so it's incomplete. The full rule is:

"Plural-Forms: nplurals=3; plural=(n%10==1 && n%100!=11 ? 0 : n%10>=2 && n"
"%10<=4 && (n%100<10 || n%100>=20) ? 1 : 2);\n"

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

#14Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: Tom Lane (#10)
Re: Remove an unnecessary errmsg_plural in dependency.c

At Thu, 24 Mar 2022 10:19:18 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:

Thanks. I think I get the point - is it dngettext doing things
differently for different languages?

Yeah. To be concrete, have a look in ru.po:

I wondered why it takes two forms of format string but I now
understand it is the fall-back texts used when translation is not
found.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#15Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: Tom Lane (#12)
Re: Remove an unnecessary errmsg_plural in dependency.c

At Thu, 24 Mar 2022 16:00:58 +0100, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in

On 2022-Mar-24, Tom Lane wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

$ git grep 'Plural-Forms' src/backend/po/*.po
ru.po:"Plural-Forms: nplurals=3; plural=(n%10==1 && n%100!=11 ? 0 : n%10>=2 && n"

Oh, interesting: if I'm reading that right, all three Russian
forms are reachable, even with the knowledge that n > 1.
(But isn't the last "&& n" test redundant?)

I wondered about that trailing 'n' and it turns out that the grep was
too simplistic, so it's incomplete. The full rule is:

"Plural-Forms: nplurals=3; plural=(n%10==1 && n%100!=11 ? 0 : n%10>=2 && n"
"%10<=4 && (n%100<10 || n%100>=20) ? 1 : 2);\n"

FWIW just for fun, I saw the first form.

postgres=# drop table t cascade;
ЗАМЕЧАНИЕ: удаление распространяется на ещё 21 объект

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center