Thread: A Japanese-unfriendy error message contruction

A Japanese-unfriendy error message contruction

From
Kyotaro HORIGUCHI
Date:
Hello.

While I revised the translation, I ran across the following code.

>    form_policy = (Form_pg_policy) GETSTRUCT(tuple);
>
>    appendStringInfo(&buffer, _("policy %s on "),
>             NameStr(form_policy->polname));
>    getRelationDescription(&buffer, form_policy->polrelid);

getRelationDescription appends a string like "table %s" to the
buffer so finally a message like "policy %s on table %s" is
constructed but this is very unfriendly to Japanese syntax.

The "policy %s" and "<objname> %s" are transposed in Japaese
syntax.  Specifically "<objname> %s NO <policy> %s" is the
natural translation of "policy %s on <objname> %s". But currently
we cannot get the natural error message in Japanese.

For the reason, I'd like to propose to refactor
getObjectDescription:OPCLASS_POLICY as the attached patch. The
same structure is seen for OPCLASS_AMOP.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index d371c47842..baa77e4e79 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -3356,6 +3356,7 @@ getObjectDescription(const ObjectAddress *object)
                 SysScanDesc sscan;
                 HeapTuple    tuple;
                 Form_pg_policy form_policy;
+                StringInfoData reldesc;
 
                 policy_rel = heap_open(PolicyRelationId, AccessShareLock);
 
@@ -3375,9 +3376,13 @@ getObjectDescription(const ObjectAddress *object)
 
                 form_policy = (Form_pg_policy) GETSTRUCT(tuple);
 
-                appendStringInfo(&buffer, _("policy %s on "),
-                                 NameStr(form_policy->polname));
-                getRelationDescription(&buffer, form_policy->polrelid);
+                initStringInfo(&reldesc);
+                getRelationDescription(&reldesc, form_policy->polrelid);
+
+                appendStringInfo(&buffer, _("policy %s on %s"),
+                                 NameStr(form_policy->polname),
+                                 reldesc.data);
+                pfree(reldesc.data);
 
                 systable_endscan(sscan);
                 heap_close(policy_rel, AccessShareLock);

Re: A Japanese-unfriendy error message contruction

From
Euler Taveira
Date:
2018-05-22 6:20 GMT-03:00 Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>:
> For the reason, I'd like to propose to refactor
> getObjectDescription:OPCLASS_POLICY as the attached patch. The
> same structure is seen for OPCLASS_AMOP.
>
+1.


-- 
   Euler Taveira                                   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento


Re: A Japanese-unfriendy error message contruction

From
Tom Lane
Date:
Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> The "policy %s" and "<objname> %s" are transposed in Japaese
> syntax.  Specifically "<objname> %s NO <policy> %s" is the
> natural translation of "policy %s on <objname> %s". But currently
> we cannot get the natural error message in Japanese.

> For the reason, I'd like to propose to refactor
> getObjectDescription:OPCLASS_POLICY as the attached patch. The
> same structure is seen for OPCLASS_AMOP.

Taking a quick look through objectaddress.c, I see quite a lot of
deficiencies:

* Is it OK for the OCLASS_CLASS-with-subId case to assume that it can
tack on "column COLNAME" after the description of the relation containing
the column?  Perhaps this is tolerable but I'm not sure.  In English it'd
be at least as plausible to write "column COLNAME of <relation>", and
maybe there are other languages where there's no good way to deal with
the current message structure.

* Column default values are written as "default for %s" where %s is what
you get from the above.  This seems likely to be even more awkward; do we
need to flatten that into one translatable string instead of recursing?
(At the very least I wish it was "default value for %s".)

* Collations have namespaces, but the OCLASS_COLLATION code doesn't
account for that.  Likewise for conversions, extended statistics
objects, and all four types of text search objects.

* OCLASS_PUBLICATION_REL likewise neglects schema-qualification of the
relation name.  I wonder why it's not using getRelationDescription, too.

* Both OCLASS_AMOP and OCLASS_AMPROC have the problem that they plug
the translation of "operator family %s for access method %s" into
"<something> of %s".  I'm not sure how big a problem this is, or whether
it's worth the code-duplication needed to expose the whole thing as
one translatable message.  Opfamily members are pretty advanced things,
so maybe we shouldn't expend too much work on them.  (But if we do,
we should also take a look at the no-such-member error strings in
get_object_address_opf_member, which do the same thing.)

* The DEFACL code appends " in schema %s" after an otherwise
translatable message.  Do we need to fix that?

* OCLASS_RULE and OCLASS_TRIGGER use the same untranslatable style
as you complain of for OCLASS_POLICY.

The missing-schema-qualification issues seem like must-fix problems to me.
But not being a translator, I'm not sure how much of a problem each of the
other issues is.  I think that there was a deliberate decision at some
point that we'd accept some translation awkwardness in this code.  How
far do we want to go to clean it up?

            regards, tom lane


Re: A Japanese-unfriendy error message contruction

From
Kyotaro HORIGUCHI
Date:
Thank you for the suggestion.

Is there anyone using another language who is having difficulties
in translating some messages?

At Tue, 22 May 2018 14:27:29 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <13575.1527013649@sss.pgh.pa.us>
> Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> > The "policy %s" and "<objname> %s" are transposed in Japaese
> > syntax.  Specifically "<objname> %s NO <policy> %s" is the
> > natural translation of "policy %s on <objname> %s". But currently
> > we cannot get the natural error message in Japanese.
> 
> > For the reason, I'd like to propose to refactor
> > getObjectDescription:OPCLASS_POLICY as the attached patch. The
> > same structure is seen for OPCLASS_AMOP.
> 
> Taking a quick look through objectaddress.c, I see quite a lot of
> deficiencies:
> 
> * Is it OK for the OCLASS_CLASS-with-subId case to assume that it can
> tack on "column COLNAME" after the description of the relation containing
> the column?  Perhaps this is tolerable but I'm not sure.  In English it'd
> be at least as plausible to write "column COLNAME of <relation>", and
> maybe there are other languages where there's no good way to deal with
> the current message structure.

In Japanese it is written as "<reltype> RELNAME 'NO' <column> COLNAME"
the or just "<reltype> RELNAME <column> COLNAME" is no problem.

> * Column default values are written as "default for %s" where %s is what
> you get from the above.  This seems likely to be even more awkward; do we
> need to flatten that into one translatable string instead of recursing?
> (At the very least I wish it was "default value for %s".)

I see this is constructed in the following structure.

| (default for ((table t) (column a))) depends on (sequence seq1)

In Japanese, a difference in this sentense is the order of the
outermost blocks so there's no problem here.

The topmost structure is "%s depends on %s" so it doesn't seem
modifiable into plnainer shape without adding duplications..

I agree that "values" make the sentense neater.

> * Collations have namespaces, but the OCLASS_COLLATION code doesn't
> account for that.  Likewise for conversions, extended statistics
> objects, and all four types of text search objects.

I'm not sure it is necessarily required. getObjectDescription
cares only for OPCLASS (and DEFACL), but we could do so for all
possible places with no difficulties.

> * OCLASS_PUBLICATION_REL likewise neglects schema-qualification of the
> relation name.  I wonder why it's not using getRelationDescription, too.

The function takes ObjectAddresss which we don't have there.
It makes the code as the following, the format string looks
somewhat confusing..

| reladdr.classId = RelationRelationId;
| reladdr.objectId = prform->prrelid;
| reladdr.objectSubId = 0;
| appendStringInfo(&buffer, _("publication %s in publication %s"),
|     getObjectDescription(&reladdr), pubname)

> * Both OCLASS_AMOP and OCLASS_AMPROC have the problem that they plug
> the translation of "operator family %s for access method %s" into
> "<something> of %s".  I'm not sure how big a problem this is, or whether
> it's worth the code-duplication needed to expose the whole thing as
> one translatable message.  Opfamily members are pretty advanced things,
> so maybe we shouldn't expend too much work on them.  (But if we do,
> we should also take a look at the no-such-member error strings in
> get_object_address_opf_member, which do the same thing.)

Regarding Japanese, it is not a problem. It is specifically the
following

| (operator %d.... of (operator family %s for access method %s))

It is translated into Japanese as:

| ((<access method> %s <for="NO"> <operator family> %s) <of="NO"> operator %d....)

# Japanese suffers here from less variation of the syntactical
# element (postpossitional particle, "NO" in the above)
# corresnponding to preposition (for and of), but it is a
# different matter.

> * The DEFACL code appends " in schema %s" after an otherwise
> translatable message.  Do we need to fix that?

Ugg! You're right. It should be moved toward the beginning of the
sentence. Looking this, I noticed that some strings that should
be translatable are forgot to be enclosed with "_()".

> * OCLASS_RULE and OCLASS_TRIGGER use the same untranslatable style
> as you complain of for OCLASS_POLICY.

# OCLASS_RULE would be OCLASS_REWRITE

Mmm. I don't remember that I saw such phrases in ja.po files but
it is surely that.

> The missing-schema-qualification issues seem like must-fix problems to me.
> But not being a translator, I'm not sure how much of a problem each of the
> other issues is.  I think that there was a deliberate decision at some
> point that we'd accept some translation awkwardness in this code.  How
> far do we want to go to clean it up?

I'll clean-up the two thinkgs and post the result later.

Thanks!

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: A Japanese-unfriendy error message contruction

From
Tom Lane
Date:
Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> At Tue, 22 May 2018 14:27:29 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <13575.1527013649@sss.pgh.pa.us>
>> * Is it OK for the OCLASS_CLASS-with-subId case to assume that it can
>> tack on "column COLNAME" after the description of the relation containing
>> the column?  Perhaps this is tolerable but I'm not sure.  In English it'd
>> be at least as plausible to write "column COLNAME of <relation>", and
>> maybe there are other languages where there's no good way to deal with
>> the current message structure.

> In Japanese it is written as "<reltype> RELNAME 'NO' <column> COLNAME"
> the or just "<reltype> RELNAME <column> COLNAME" is no problem.

After thinking about this some more, I'd like to propose that we change
the English output to be "column COLNAME of <relation>", using code
similar to what you suggested for O_POLICY etc.  I know that I've been
momentarily confused more than once by looking at obj_description output
and thinking "what, the whole relation depends on this? ... oh, no, it's
just the one column".  It would be better if the head-word were "column".
If that leads to better translations in other languages, fine, but in
any case this'd be an improvement for English.

> I'll clean-up the two thinkgs and post the result later.

OK, I'll await your patch before doing more here.

            regards, tom lane


Re: A Japanese-unfriendy error message contruction

From
Kyotaro HORIGUCHI
Date:
Hello. Here is the patch set.

At Wed, 23 May 2018 11:20:24 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <4979.1527088824@sss.pgh.pa.us>
> After thinking about this some more, I'd like to propose that we change
> the English output to be "column COLNAME of <relation>", using code
> similar to what you suggested for O_POLICY etc.  I know that I've been
> momentarily confused more than once by looking at obj_description output
> and thinking "what, the whole relation depends on this? ... oh, no, it's
> just the one column".  It would be better if the head-word were "column".
> If that leads to better translations in other languages, fine, but in
> any case this'd be an improvement for English.
> 
> > I'll clean-up the two thinkgs and post the result later.
> 
> OK, I'll await your patch before doing more here.

Constraints also have namespace but I understand it is just a
decoration so I left it alone.

1. Remove tranlation obstracles.
2. Show qualified names for all possible object types.
3. Changes "relation R column C" to "column C of relation R".

Extended statistics's name qualification is not excercised in the
current regression test.

I found that the last one you sugeested makes error messages far
cleaner, easy to grasp the meaning at a glance.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From cdb956ac2da784f37b110f61da8915819d3ff107 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 24 May 2018 19:23:40 +0900
Subject: [PATCH 1/3] Make object names more translatable

getObjectDescription() is making description for some object classes
in a fashon that is hard-to-translate into some languages. This patch
changes it easier to translate. This changes "default" for "default
value" along with that.
---
 src/backend/catalog/objectaddress.c    | 74 ++++++++++++++++++++++++++--------
 src/test/regress/expected/sequence.out |  4 +-
 2 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index d371c47842..3b27ed7cf4 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2829,7 +2829,7 @@ getObjectDescription(const ObjectAddress *object)
                 colobject.objectId = attrdef->adrelid;
                 colobject.objectSubId = attrdef->adnum;
 
-                appendStringInfo(&buffer, _("default for %s"),
+                appendStringInfo(&buffer, _("default value for %s"),
                                  getObjectDescription(&colobject));
 
                 systable_endscan(adscan);
@@ -3016,6 +3016,7 @@ getObjectDescription(const ObjectAddress *object)
                 SysScanDesc rcscan;
                 HeapTuple    tup;
                 Form_pg_rewrite rule;
+                StringInfoData reldesc;
 
                 ruleDesc = heap_open(RewriteRelationId, AccessShareLock);
 
@@ -3035,9 +3036,17 @@ getObjectDescription(const ObjectAddress *object)
 
                 rule = (Form_pg_rewrite) GETSTRUCT(tup);
 
-                appendStringInfo(&buffer, _("rule %s on "),
-                                 NameStr(rule->rulename));
-                getRelationDescription(&buffer, rule->ev_class);
+                /*
+                 * You might think this could be written simpler, but we
+                 * should give a chance to reorder the words in translation.
+                 */
+                initStringInfo(&reldesc);
+                getRelationDescription(&reldesc, rule->ev_class);
+
+                appendStringInfo(&buffer, _("rule %s on %s"),
+                                 NameStr(rule->rulename),
+                                 reldesc.data);
+                pfree(reldesc.data);
 
                 systable_endscan(rcscan);
                 heap_close(ruleDesc, AccessShareLock);
@@ -3051,6 +3060,7 @@ getObjectDescription(const ObjectAddress *object)
                 SysScanDesc tgscan;
                 HeapTuple    tup;
                 Form_pg_trigger trig;
+                StringInfoData reldesc;
 
                 trigDesc = heap_open(TriggerRelationId, AccessShareLock);
 
@@ -3070,9 +3080,17 @@ getObjectDescription(const ObjectAddress *object)
 
                 trig = (Form_pg_trigger) GETSTRUCT(tup);
 
-                appendStringInfo(&buffer, _("trigger %s on "),
-                                 NameStr(trig->tgname));
-                getRelationDescription(&buffer, trig->tgrelid);
+                /*
+                 * You might think this could be written simpler, but we
+                 * should give a chance to reorder the words in translation.
+                 */
+                initStringInfo(&reldesc);
+                getRelationDescription(&reldesc, trig->tgrelid);
+
+                appendStringInfo(&buffer, _("trigger %s on %s"),
+                                 NameStr(trig->tgname),
+                                 reldesc.data);
+                pfree(reldesc.data);
 
                 systable_endscan(tgscan);
                 heap_close(trigDesc, AccessShareLock);
@@ -3256,6 +3274,8 @@ getObjectDescription(const ObjectAddress *object)
                 SysScanDesc rcscan;
                 HeapTuple    tup;
                 Form_pg_default_acl defacl;
+                StringInfo        acldesc = &buffer;
+                StringInfoData    tmpbuf;
 
                 defaclrel = heap_open(DefaultAclRelationId, AccessShareLock);
 
@@ -3275,36 +3295,43 @@ getObjectDescription(const ObjectAddress *object)
 
                 defacl = (Form_pg_default_acl) GETSTRUCT(tup);
 
+                if (OidIsValid(defacl->defaclnamespace))
+                {
+                    /* We combine acldesc with schema name later */
+                    acldesc = &tmpbuf;
+                    initStringInfo(acldesc);
+                }
+
                 switch (defacl->defaclobjtype)
                 {
                     case DEFACLOBJ_RELATION:
-                        appendStringInfo(&buffer,
+                        appendStringInfo(acldesc,
                                          _("default privileges on new relations belonging to role %s"),
                                          GetUserNameFromId(defacl->defaclrole, false));
                         break;
                     case DEFACLOBJ_SEQUENCE:
-                        appendStringInfo(&buffer,
+                        appendStringInfo(acldesc,
                                          _("default privileges on new sequences belonging to role %s"),
                                          GetUserNameFromId(defacl->defaclrole, false));
                         break;
                     case DEFACLOBJ_FUNCTION:
-                        appendStringInfo(&buffer,
+                        appendStringInfo(acldesc,
                                          _("default privileges on new functions belonging to role %s"),
                                          GetUserNameFromId(defacl->defaclrole, false));
                         break;
                     case DEFACLOBJ_TYPE:
-                        appendStringInfo(&buffer,
+                        appendStringInfo(acldesc,
                                          _("default privileges on new types belonging to role %s"),
                                          GetUserNameFromId(defacl->defaclrole, false));
                         break;
                     case DEFACLOBJ_NAMESPACE:
-                        appendStringInfo(&buffer,
+                        appendStringInfo(acldesc,
                                          _("default privileges on new schemas belonging to role %s"),
                                          GetUserNameFromId(defacl->defaclrole, false));
                         break;
                     default:
                         /* shouldn't get here */
-                        appendStringInfo(&buffer,
+                        appendStringInfo(acldesc,
                                          _("default privileges belonging to role %s"),
                                          GetUserNameFromId(defacl->defaclrole, false));
                         break;
@@ -3312,9 +3339,17 @@ getObjectDescription(const ObjectAddress *object)
 
                 if (OidIsValid(defacl->defaclnamespace))
                 {
+                    /*
+                     * You might think this could be written simpler, but we
+                     * should give a chance to reorder the words in
+                     * translation.
+                     */
+                    Assert(&buffer != acldesc);
                     appendStringInfo(&buffer,
-                                     _(" in schema %s"),
+                                     _("%s in schema %s"),
+                                     acldesc->data,
                                      get_namespace_name(defacl->defaclnamespace));
+                    pfree(acldesc->data);
                 }
 
                 systable_endscan(rcscan);
@@ -3356,6 +3391,7 @@ getObjectDescription(const ObjectAddress *object)
                 SysScanDesc sscan;
                 HeapTuple    tuple;
                 Form_pg_policy form_policy;
+                StringInfoData reldesc;
 
                 policy_rel = heap_open(PolicyRelationId, AccessShareLock);
 
@@ -3375,9 +3411,13 @@ getObjectDescription(const ObjectAddress *object)
 
                 form_policy = (Form_pg_policy) GETSTRUCT(tuple);
 
-                appendStringInfo(&buffer, _("policy %s on "),
-                                 NameStr(form_policy->polname));
-                getRelationDescription(&buffer, form_policy->polrelid);
+                initStringInfo(&reldesc);
+                getRelationDescription(&reldesc, form_policy->polrelid);
+
+                appendStringInfo(&buffer, _("policy %s on %s"),
+                                 NameStr(form_policy->polname),
+                                 reldesc.data);
+                pfree(reldesc.data);
 
                 systable_endscan(sscan);
                 heap_close(policy_rel, AccessShareLock);
diff --git a/src/test/regress/expected/sequence.out b/src/test/regress/expected/sequence.out
index ca5ea063fa..f2e20a7e85 100644
--- a/src/test/regress/expected/sequence.out
+++ b/src/test/regress/expected/sequence.out
@@ -293,11 +293,11 @@ CREATE TEMP TABLE t1 (
 -- Both drops should fail, but with different error messages:
 DROP SEQUENCE t1_f1_seq;
 ERROR:  cannot drop sequence t1_f1_seq because other objects depend on it
-DETAIL:  default for table t1 column f1 depends on sequence t1_f1_seq
+DETAIL:  default value for table t1 column f1 depends on sequence t1_f1_seq
 HINT:  Use DROP ... CASCADE to drop the dependent objects too.
 DROP SEQUENCE myseq2;
 ERROR:  cannot drop sequence myseq2 because other objects depend on it
-DETAIL:  default for table t1 column f2 depends on sequence myseq2
+DETAIL:  default value for table t1 column f2 depends on sequence myseq2
 HINT:  Use DROP ... CASCADE to drop the dependent objects too.
 -- This however will work:
 DROP SEQUENCE myseq3;
-- 
2.16.3

From 2a57b2edbd814ca2609ec9fb1efd99d7f626c694 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 24 May 2018 21:06:04 +0900
Subject: [PATCH 2/3] Qualify name of all objects that can define namespace

Name of some kinds of object were missing qualification in
getObjectDescription. This patch make them properly qualified.
---
 src/backend/catalog/objectaddress.c       | 90 ++++++++++++++++++++++++++++---
 src/test/regress/expected/alter_table.out | 10 ++--
 2 files changed, 88 insertions(+), 12 deletions(-)

diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 3b27ed7cf4..98850df39c 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2738,6 +2738,7 @@ getObjectDescription(const ObjectAddress *object)
             {
                 HeapTuple    collTup;
                 Form_pg_collation coll;
+                char       *nspname;
 
                 collTup = SearchSysCache1(COLLOID,
                                           ObjectIdGetDatum(object->objectId));
@@ -2745,8 +2746,16 @@ getObjectDescription(const ObjectAddress *object)
                     elog(ERROR, "cache lookup failed for collation %u",
                          object->objectId);
                 coll = (Form_pg_collation) GETSTRUCT(collTup);
+
+                /* Qualify the name if not visible in search path */
+                if (CollationIsVisible(object->objectId))
+                    nspname = NULL;
+                else
+                    nspname = get_namespace_name(coll->collnamespace);
+
                 appendStringInfo(&buffer, _("collation %s"),
-                                 NameStr(coll->collname));
+                                 quote_qualified_identifier(nspname,
+                                                            NameStr(coll->collname)));
                 ReleaseSysCache(collTup);
                 break;
             }
@@ -2786,14 +2795,25 @@ getObjectDescription(const ObjectAddress *object)
         case OCLASS_CONVERSION:
             {
                 HeapTuple    conTup;
+                Form_pg_conversion conv;
+                char       *nspname;
 
                 conTup = SearchSysCache1(CONVOID,
                                          ObjectIdGetDatum(object->objectId));
                 if (!HeapTupleIsValid(conTup))
                     elog(ERROR, "cache lookup failed for conversion %u",
                          object->objectId);
+                conv = (Form_pg_conversion) GETSTRUCT(conTup);
+                
+                /* Qualify the name if not visible in search path */
+                if (ConversionIsVisible(object->objectId))
+                    nspname = NULL;
+                else
+                    nspname = get_namespace_name(conv->connamespace);
+
                 appendStringInfo(&buffer, _("conversion %s"),
-                                 NameStr(((Form_pg_conversion) GETSTRUCT(conTup))->conname));
+                                 quote_qualified_identifier(nspname,
+                                                            NameStr(conv->conname)));
                 ReleaseSysCache(conTup);
                 break;
             }
@@ -3113,6 +3133,7 @@ getObjectDescription(const ObjectAddress *object)
             {
                 HeapTuple    stxTup;
                 Form_pg_statistic_ext stxForm;
+                char       *nspname;
 
                 stxTup = SearchSysCache1(STATEXTOID,
                                          ObjectIdGetDatum(object->objectId));
@@ -3122,8 +3143,15 @@ getObjectDescription(const ObjectAddress *object)
 
                 stxForm = (Form_pg_statistic_ext) GETSTRUCT(stxTup);
 
+                /* Qualify the name if not visible in search path */
+                if (StatisticsObjIsVisible(object->objectId))
+                    nspname = NULL;
+                else
+                    nspname = get_namespace_name(stxForm->stxnamespace);
+                
                 appendStringInfo(&buffer, _("statistics object %s"),
-                                 NameStr(stxForm->stxname));
+                                 quote_qualified_identifier(nspname,
+                                                            NameStr(stxForm->stxname)));
 
                 ReleaseSysCache(stxTup);
                 break;
@@ -3132,14 +3160,26 @@ getObjectDescription(const ObjectAddress *object)
         case OCLASS_TSPARSER:
             {
                 HeapTuple    tup;
+                Form_pg_ts_parser prsForm;
+                char       *nspname;
 
                 tup = SearchSysCache1(TSPARSEROID,
                                       ObjectIdGetDatum(object->objectId));
                 if (!HeapTupleIsValid(tup))
                     elog(ERROR, "cache lookup failed for text search parser %u",
                          object->objectId);
+
+                prsForm = (Form_pg_ts_parser) GETSTRUCT(tup);
+
+                /* Qualify the name if not visible in search path */
+                if (TSParserIsVisible(object->objectId))
+                    nspname = NULL;
+                else
+                    nspname = get_namespace_name(prsForm->prsnamespace);
+
                 appendStringInfo(&buffer, _("text search parser %s"),
-                                 NameStr(((Form_pg_ts_parser) GETSTRUCT(tup))->prsname));
+                                 quote_qualified_identifier(nspname,
+                                                            NameStr(prsForm->prsname)));
                 ReleaseSysCache(tup);
                 break;
             }
@@ -3147,14 +3187,26 @@ getObjectDescription(const ObjectAddress *object)
         case OCLASS_TSDICT:
             {
                 HeapTuple    tup;
+                Form_pg_ts_dict dictForm;
+                char       *nspname;
 
                 tup = SearchSysCache1(TSDICTOID,
                                       ObjectIdGetDatum(object->objectId));
                 if (!HeapTupleIsValid(tup))
                     elog(ERROR, "cache lookup failed for text search dictionary %u",
                          object->objectId);
+
+                dictForm = (Form_pg_ts_dict) GETSTRUCT(tup);
+
+                /* Qualify the name if not visible in search path */
+                if (TSDictionaryIsVisible(object->objectId))
+                    nspname = NULL;
+                else
+                    nspname = get_namespace_name(dictForm->dictnamespace);
+
                 appendStringInfo(&buffer, _("text search dictionary %s"),
-                                 NameStr(((Form_pg_ts_dict) GETSTRUCT(tup))->dictname));
+                                 quote_qualified_identifier(nspname,
+                                                            NameStr(dictForm->dictname)));
                 ReleaseSysCache(tup);
                 break;
             }
@@ -3162,14 +3214,26 @@ getObjectDescription(const ObjectAddress *object)
         case OCLASS_TSTEMPLATE:
             {
                 HeapTuple    tup;
+                Form_pg_ts_template tmplForm;
+                char       *nspname;
 
                 tup = SearchSysCache1(TSTEMPLATEOID,
                                       ObjectIdGetDatum(object->objectId));
                 if (!HeapTupleIsValid(tup))
                     elog(ERROR, "cache lookup failed for text search template %u",
                          object->objectId);
+
+                tmplForm = (Form_pg_ts_template) GETSTRUCT(tup);
+
+                /* Qualify the name if not visible in search path */
+                if (TSTemplateIsVisible(object->objectId))
+                    nspname = NULL;
+                else
+                    nspname = get_namespace_name(tmplForm->tmplnamespace);
+
                 appendStringInfo(&buffer, _("text search template %s"),
-                                 NameStr(((Form_pg_ts_template) GETSTRUCT(tup))->tmplname));
+                                 quote_qualified_identifier(nspname,
+                                                            NameStr(tmplForm->tmplname)));
                 ReleaseSysCache(tup);
                 break;
             }
@@ -3177,14 +3241,26 @@ getObjectDescription(const ObjectAddress *object)
         case OCLASS_TSCONFIG:
             {
                 HeapTuple    tup;
+                Form_pg_ts_config cfgForm;
+                char       *nspname;
 
                 tup = SearchSysCache1(TSCONFIGOID,
                                       ObjectIdGetDatum(object->objectId));
                 if (!HeapTupleIsValid(tup))
                     elog(ERROR, "cache lookup failed for text search configuration %u",
                          object->objectId);
+
+                cfgForm = (Form_pg_ts_config) GETSTRUCT(tup);
+
+                /* Qualify the name if not visible in search path */
+                if (TSConfigIsVisible(object->objectId))
+                    nspname = NULL;
+                else
+                    nspname = get_namespace_name(cfgForm->cfgnamespace);
+
                 appendStringInfo(&buffer, _("text search configuration %s"),
-                                 NameStr(((Form_pg_ts_config) GETSTRUCT(tup))->cfgname));
+                                 quote_qualified_identifier(nspname,
+                                                            NameStr(cfgForm->cfgname)));
                 ReleaseSysCache(tup);
                 break;
             }
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 50b9443e2d..376194c48a 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2680,11 +2680,11 @@ drop cascades to operator family alter2.ctype_hash_ops for access method hash
 drop cascades to type alter2.ctype
 drop cascades to function alter2.same(alter2.ctype,alter2.ctype)
 drop cascades to operator alter2.=(alter2.ctype,alter2.ctype)
-drop cascades to conversion ascii_to_utf8
-drop cascades to text search parser prs
-drop cascades to text search configuration cfg
-drop cascades to text search template tmpl
-drop cascades to text search dictionary dict
+drop cascades to conversion alter2.ascii_to_utf8
+drop cascades to text search parser alter2.prs
+drop cascades to text search configuration alter2.cfg
+drop cascades to text search template alter2.tmpl
+drop cascades to text search dictionary alter2.dict
 --
 -- composite types
 --
-- 
2.16.3

From 8eda8d29e1fdfeaed9fde6f12967416aecfb6449 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 24 May 2018 21:35:10 +0900
Subject: [PATCH 3/3] Change object description of columns

Previously columns were descripted as "relation R column C", but it
sometimes confusing that it reads as a relation R, not a column
C. This patch transposes the description as "column C of relation R".
---
 src/backend/catalog/objectaddress.c       | 16 +++++++++++++---
 src/test/regress/expected/alter_table.out |  6 +++---
 src/test/regress/expected/collate.out     |  2 +-
 src/test/regress/expected/domain.out      |  8 ++++----
 src/test/regress/expected/sequence.out    |  4 ++--
 src/test/regress/expected/triggers.out    |  8 ++++----
 6 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 98850df39c..61fe283d74 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2681,12 +2681,22 @@ getObjectDescription(const ObjectAddress *object)
     switch (getObjectClass(object))
     {
         case OCLASS_CLASS:
-            getRelationDescription(&buffer, object->objectId);
             if (object->objectSubId != 0)
-                appendStringInfo(&buffer, _(" column %s"),
+            {
+                StringInfoData reldesc;
+
+                initStringInfo(&reldesc);
+                getRelationDescription(&reldesc, object->objectId);
+                appendStringInfo(&buffer, _("column %s of %s"),
                                  get_attname(object->objectId,
                                              object->objectSubId,
-                                             false));
+                                             false),
+                                 reldesc.data);
+                pfree(reldesc.data);
+            }
+            else
+                getRelationDescription(&buffer, object->objectId);
+
             break;
 
         case OCLASS_PROC:
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 376194c48a..702bf9fe98 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1844,7 +1844,7 @@ select * from foo;
 (1 row)
 
 drop domain mytype cascade;
-NOTICE:  drop cascades to table foo column f2
+NOTICE:  drop cascades to column f2 of table foo
 select * from foo;
  f1 | f3 
 ----+----
@@ -2870,8 +2870,8 @@ DROP TABLE test_tbl2_subclass;
 CREATE TYPE test_typex AS (a int, b text);
 CREATE TABLE test_tblx (x int, y test_typex check ((y).a > 0));
 ALTER TYPE test_typex DROP ATTRIBUTE a; -- fails
-ERROR:  cannot drop composite type test_typex column a because other objects depend on it
-DETAIL:  constraint test_tblx_y_check on table test_tblx depends on composite type test_typex column a
+ERROR:  cannot drop column a of composite type test_typex because other objects depend on it
+DETAIL:  constraint test_tblx_y_check on table test_tblx depends on column a of composite type test_typex
 HINT:  Use DROP ... CASCADE to drop the dependent objects too.
 ALTER TYPE test_typex DROP ATTRIBUTE a CASCADE;
 NOTICE:  drop cascades to constraint test_tblx_y_check on table test_tblx
diff --git a/src/test/regress/expected/collate.out b/src/test/regress/expected/collate.out
index f045f2b291..fcbe3a5cc8 100644
--- a/src/test/regress/expected/collate.out
+++ b/src/test/regress/expected/collate.out
@@ -631,7 +631,7 @@ DROP COLLATION mycoll1;
 CREATE TABLE collate_test23 (f1 text collate mycoll2);
 DROP COLLATION mycoll2;  -- fail
 ERROR:  cannot drop collation mycoll2 because other objects depend on it
-DETAIL:  table collate_test23 column f1 depends on collation mycoll2
+DETAIL:  column f1 of table collate_test23 depends on collation mycoll2
 HINT:  Use DROP ... CASCADE to drop the dependent objects too.
 -- invalid: non-lowercase quoted identifiers
 CREATE COLLATION case_coll ("Lc_Collate" = "POSIX", "Lc_Ctype" = "POSIX");
diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index f4eebb75cf..0b5a9041b0 100644
--- a/src/test/regress/expected/domain.out
+++ b/src/test/regress/expected/domain.out
@@ -298,8 +298,8 @@ ERROR:  operator does not exist: character varying > double precision
 HINT:  No operator matches the given name and argument types. You might need to add explicit type casts.
 alter type comptype alter attribute r type bigint;
 alter type comptype drop attribute r;  -- fail
-ERROR:  cannot drop composite type comptype column r because other objects depend on it
-DETAIL:  constraint c1 depends on composite type comptype column r
+ERROR:  cannot drop column r of composite type comptype because other objects depend on it
+DETAIL:  constraint c1 depends on column r of composite type comptype
 HINT:  Use DROP ... CASCADE to drop the dependent objects too.
 alter type comptype drop attribute i;
 select conname, obj_description(oid, 'pg_constraint') from pg_constraint
@@ -645,8 +645,8 @@ alter domain dnotnulltest drop not null;
 update domnotnull set col1 = null;
 drop domain dnotnulltest cascade;
 NOTICE:  drop cascades to 2 other objects
-DETAIL:  drop cascades to table domnotnull column col1
-drop cascades to table domnotnull column col2
+DETAIL:  drop cascades to column col1 of table domnotnull
+drop cascades to column col2 of table domnotnull
 -- Test ALTER DOMAIN .. DEFAULT ..
 create table domdeftest (col1 ddef1);
 insert into domdeftest default values;
diff --git a/src/test/regress/expected/sequence.out b/src/test/regress/expected/sequence.out
index f2e20a7e85..a0d2b22d3c 100644
--- a/src/test/regress/expected/sequence.out
+++ b/src/test/regress/expected/sequence.out
@@ -293,11 +293,11 @@ CREATE TEMP TABLE t1 (
 -- Both drops should fail, but with different error messages:
 DROP SEQUENCE t1_f1_seq;
 ERROR:  cannot drop sequence t1_f1_seq because other objects depend on it
-DETAIL:  default value for table t1 column f1 depends on sequence t1_f1_seq
+DETAIL:  default value for column f1 of table t1 depends on sequence t1_f1_seq
 HINT:  Use DROP ... CASCADE to drop the dependent objects too.
 DROP SEQUENCE myseq2;
 ERROR:  cannot drop sequence myseq2 because other objects depend on it
-DETAIL:  default value for table t1 column f2 depends on sequence myseq2
+DETAIL:  default value for column f2 of table t1 depends on sequence myseq2
 HINT:  Use DROP ... CASCADE to drop the dependent objects too.
 -- This however will work:
 DROP SEQUENCE myseq3;
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 1f8caef2d7..bf271d536e 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -557,10 +557,10 @@ LINE 2: FOR EACH STATEMENT WHEN (OLD.* IS DISTINCT FROM NEW.*)
                                  ^
 -- check dependency restrictions
 ALTER TABLE main_table DROP COLUMN b;
-ERROR:  cannot drop table main_table column b because other objects depend on it
-DETAIL:  trigger after_upd_b_row_trig on table main_table depends on table main_table column b
-trigger after_upd_a_b_row_trig on table main_table depends on table main_table column b
-trigger after_upd_b_stmt_trig on table main_table depends on table main_table column b
+ERROR:  cannot drop column b of table main_table because other objects depend on it
+DETAIL:  trigger after_upd_b_row_trig on table main_table depends on column b of table main_table
+trigger after_upd_a_b_row_trig on table main_table depends on column b of table main_table
+trigger after_upd_b_stmt_trig on table main_table depends on column b of table main_table
 HINT:  Use DROP ... CASCADE to drop the dependent objects too.
 -- this should succeed, but we'll roll it back to keep the triggers around
 begin;
-- 
2.16.3


Re: A Japanese-unfriendy error message contruction

From
Tom Lane
Date:
Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> Hello. Here is the patch set.

Thanks!  I pushed all this plus fixes for the OCLASS_PUBLICATION_REL
code.

The only non-cosmetic change I made was that I didn't much like what
you'd done with the OCLASS_DEFACL code: it seemed quite confusing to
be renaming buffers like that, plus it was still assuming more than
I thought it should about how the message would go together in the end.
I took the advice of the manual instead, and just made pairs of
near-duplicate messages to be translated separately.

I'm still not sure whether the OCLASS_DEFAULT case is satisfactorily
translatable.  It would not take a lot more code to expand its message to
"default value for column %s of %s", where the second %s is the result of
getRelationDescription.  However, I have no idea whether there are any
translations where that would really work noticeably better.

            regards, tom lane


Re: A Japanese-unfriendy error message contruction

From
Kyotaro HORIGUCHI
Date:
At Thu, 24 May 2018 14:20:21 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <24988.1527186021@sss.pgh.pa.us>
> Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> > Hello. Here is the patch set.
> 
> Thanks!  I pushed all this plus fixes for the OCLASS_PUBLICATION_REL
> code.

Thanks.

> The only non-cosmetic change I made was that I didn't much like what
> you'd done with the OCLASS_DEFACL code: it seemed quite confusing to
> be renaming buffers like that, plus it was still assuming more than
> I thought it should about how the message would go together in the end.
> I took the advice of the manual instead, and just made pairs of
> near-duplicate messages to be translated separately.

I wasn't sure that which way is better. This doubles the
msgids. But I can live with it since it would be scaecely
changed.

> I'm still not sure whether the OCLASS_DEFAULT case is satisfactorily
> translatable.  It would not take a lot more code to expand its message to
> "default value for column %s of %s", where the second %s is the result of
> getRelationDescription.  However, I have no idea whether there are any
> translations where that would really work noticeably better.

True. I think we don't have difficulties as for Japanese
translation.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center