Re: Different gettext domain needed for error context

I just noticed that we use the same gettext domain for all messages
attached to one error. That is wrong in case of context information,
where you have a stack of context lines, originating from different
modules. The result is that context lines don't always get translated.

For example:

postgres=# set lc_messages ='de_DE.utf8';
SET
postgres=# do $$
begin
select 1 / 0;
end
$$;
FEHLER: Division durch Null
CONTEXT: SQL-Anweisung »select 1 / 0«
PL/pgSQL function "inline_code_block" line 3 at SQL-Anweisung

Notice how the string "PL/pgSQL function ..." is not translated. The
ereport call that raises that error is in int4div, which is in the
backend gettext domain, "postgres". But the errcontext() call comes from
pl_exec.c.

If the error originates from src/pl/plpgsql, then it works:

postgres=# do $$
begin
raise;
end
$$;
FEHLER: RAISE ohne Parameter kann nicht außerhalb einer
Ausnahmebehandlung verwendet werden
CONTEXT: PL/pgSQL-Funktion »inline_code_block« Zeile 3 bei RAISE

In theory, I guess the other fields like errhint() potentially have the
same problem, but they're not currently used to provide extra
information to messages originating from another module, so I'm inclined
to leave them alone for now.

To fix this, we need to somehow pass the caller's text domain to
errcontext(). The most straightforward way is to pass it as an extra
argument. Ideally, errcontext() would be a macro that passes TEXTDOMAIN
to the underlying function, so that you don't need to change all the
callers of errcontext():

#define errcontext(...) errcontext_domain(TEXTDOMAIN, ...)

But that doesn't work, because it would require varags macros. Anyone
know a trick to make that work?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> To fix this, we need to somehow pass the caller's text domain to
> errcontext(). The most straightforward way is to pass it as an extra
> argument. Ideally, errcontext() would be a macro that passes TEXTDOMAIN
> to the underlying function, so that you don't need to change all the
> callers of errcontext():

> #define errcontext(...) errcontext_domain(TEXTDOMAIN, ...)

> But that doesn't work, because it would require varags macros. Anyone
> know a trick to make that work?

This is pretty ugly, but AFAICS it should work:

#define errcontext set_errcontext_domain(TEXTDOMAIN), real_errcontext

But it might be better in the long run to bite the bullet and make
people change all their errcontext calls. There aren't that many,
especially not outside the core code.

regards, tom lane

On 15.02.2012 20:13, Tom Lane wrote:
> Heikki Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
>> To fix this, we need to somehow pass the caller's text domain to
>> errcontext(). The most straightforward way is to pass it as an extra
>> argument. Ideally, errcontext() would be a macro that passes TEXTDOMAIN
>> to the underlying function, so that you don't need to change all the
>> callers of errcontext():
>
>> #define errcontext(...) errcontext_domain(TEXTDOMAIN, ...)
>
>> But that doesn't work, because it would require varags macros. Anyone
>> know a trick to make that work?
>
> This is pretty ugly, but AFAICS it should work:
>
> #define errcontext set_errcontext_domain(TEXTDOMAIN), real_errcontext
>
> But it might be better in the long run to bite the bullet and make
> people change all their errcontext calls. There aren't that many,
> especially not outside the core code.

Agreed, and not many of the external modules are translated, anyway.

I played with a few different approaches:

1. Add a variant of errcontext() that takes a text domain argument, so
that the calls look like:

errcontext_domain(TEXTDOMAIN, "PL/Perl anonymous code block");

Straightforward, but it looks silly to have to pass TEXTDOMAIN as an
explicit argument. It's not usually explicitly used in C code at all.

2. Add a new field to ErrorContextCallback struct, indicating the
domain. So to set up a callback, you'd do:

ErrorContextCallback pl_error_context;

/* Set up a callback for error reporting */
pl_error_context.callback = plperl_inline_callback;
pl_error_context.previous = error_context_stack;
pl_error_context.arg = (Datum) 0;
pl_error_context.domain = TEXTDOMAIN;
error_context_stack = &pl_error_context;

A variant of this is to encapsulate that boilerplate code to a new macro
or function, so that you'd do just:

push_error_context(&pl_err_context, plperl_inline_callback, (Datum) 0);

push_error_context macro would automatically set the domain to
TEXTDOMAIN, similar to what ereport() does.

A problem with this approach is that if we add a new field to the
struct, any existing code would leave it uninitialized. That could
easily lead to mysterious crashes.

3. In the callback function, call a new function to set the domain to be
used for the errcontext() calls in that callback:

/* use the right domain to translate the errcontext() calls */
set_errtextdomain();

errcontext("PL/Perl anonymous code block");

Attached is a patch using this approach, I like it the most. Existing
code still works, as far as it works today, and it's easy to add that
set_errtextdomain() call to fix callbacks that have translated message.

Barring objections, I'll commit this.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> 3. In the callback function, call a new function to set the domain to be
> used for the errcontext() calls in that callback:

> /* use the right domain to translate the errcontext() calls */
> set_errtextdomain();

> errcontext("PL/Perl anonymous code block");

> Attached is a patch using this approach, I like it the most.

I don't like it at all. It seems horridly error-prone to me: there
*will* be sins of omission all over the place.

I really think we need to change errcontext itself to pass the correct
domain. If we are going to require a domain to be provided (and this
does require that, for correct operation), then we need to break any
code that doesn't provide it in a visible fashion.

A possibly more attractive alternative is to redefine errcontext
with a macro that allows TEXTDOMAIN to be passed in behind-the-scenes,
thus keeping source-level compatibility. We can do this with the same
type of hack we've used for many years for elog():

#define errcontext set_errcontext_domain(TEXTDOMAIN), errcontext_msg

where the actual message-passing function is now called errcontext_msg.

regards, tom lane

On 15.04.2012 00:54, Tom Lane wrote:
> I really think we need to change errcontext itself to pass the correct
> domain. If we are going to require a domain to be provided (and this
> does require that, for correct operation), then we need to break any
> code that doesn't provide it in a visible fashion.
>
> A possibly more attractive alternative is to redefine errcontext
> with a macro that allows TEXTDOMAIN to be passed in behind-the-scenes,
> thus keeping source-level compatibility. We can do this with the same
> type of hack we've used for many years for elog():
>
> #define errcontext set_errcontext_domain(TEXTDOMAIN), errcontext_msg
>
> where the actual message-passing function is now called errcontext_msg.

Ok then, here's a patch using that approach.

I had to rename a few local variables called "errcontext", because the
macro now tries to expands those and you get an error.

Note: If you want to test this at home, the original test case I posted
doesn't currently work because the text of the context messages in
PL/pgSQL have been slightly changed since I posted the original test
case, but the translations have not been updated yet. Until then, you
can manually remove the double quotes in messages like 'function \"%s\"'
in the .po file to test this.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com