Thread: Remove an unnecessary errmsg_plural in dependency.c

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.

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.

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

> 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/

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.

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.

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.

> 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/

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.

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

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)

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

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/

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

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