Re: [HACKERS] Different gettext domain needed for error context

[Heikki Linnakangas]

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
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 0c301b2..a10b569 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6504,7 +6504,7 @@ StartupXLOG(void)
 			bool		recoveryContinue = true;
 			bool		recoveryApply = true;
 			bool		recoveryPause = false;
-			ErrorContextCallback errcontext;
+			ErrorContextCallback errcallback;
 			TimestampTz xtime;
 
 			InRedo = true;
@@ -6566,10 +6566,10 @@ StartupXLOG(void)
 }
 
 /* Setup error traceback support for ereport() */
-errcontext.callback = rm_redo_error_callback;
-errcontext.arg = (void *) record;
-errcontext.previous = error_context_stack;
-error_context_stack = errcontext;
+errcallback.callback = rm_redo_error_callback;
+errcallback.arg = (void *) record;
+errcallback.previous = error_context_stack;
+error_context_stack = errcallback;
 
 /*
  * ShmemVariableCache-nextXid must be beyond record's xid.
@@ -6614,7 +6614,7 @@ StartupXLOG(void)
 RmgrTable[record-xl_rmid].rm_redo(EndRecPtr, record);
 
 /* Pop the error context stack */
-error_context_stack = errcontext.previous;
+error_context_stack = errcallback.previous;
 
 if (!XLogRecPtrIsInvalid(ControlFile-backupStartPoint) 
 	XLByteLE(ControlFile-backupEndPoint, EndRecPtr))
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 95fec8d..cb7e67a 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1854,7 +1854,7 @@ CopyFrom(CopyState cstate)
 	TupleTableSlot *myslot;
 	MemoryContext oldcontext = CurrentMemoryContext;
 
-	ErrorContextCallback errcontext;
+	ErrorContextCallback errcallback;
 	CommandId	mycid = GetCurrentCommandId(true);
 	int			hi_options = 0; /* start with default heap_insert options */
 	BulkInsertState bistate;
@@ -1998,10 +1998,10 @@ CopyFrom(CopyState cstate)
 	econtext = GetPerTupleExprContext(estate);
 
 	/* Set up callback to identify error line number */
-	errcontext.callback = CopyFromErrorCallback;
-	errcontext.arg = (void *) cstate;
-	errcontext.previous = error_context_stack;
-	error_context_stack = errcontext;
+	errcallback.callback = CopyFromErrorCallback;
+	errcallback.arg = (void *) cstate;
+	errcallback.previous = error_context_stack;
+	error_context_stack = errcallback;
 
 	for (;;)
 	{
@@ -2116,7 +2116,7 @@ CopyFrom(CopyState cstate)
 			nBufferedTuples, bufferedTuples);
 
 	/* Done, clean up */
-	error_context_stack = errcontext.previous;
+	error_context_stack = errcallback.previous;
 
 	FreeBulkInsertState(bistate);
 
diff --git a/src/backend/parser/parse_node.c b/src/backend/parser/parse_node.c
index 80dbdd1..33966ee 100644
--- a/src/backend/parser/parse_node.c
+++ b/src/backend/parser/parse_node.c
@@ -144,10 +144,10 @@ setup_parser_errposition_callback(ParseCallbackState *pcbstate,
 	/* Setup error traceback support for ereport() */
 	pcbstate-pstate = pstate;
 	pcbstate-location = location;
-	pcbstate-errcontext.callback = pcb_error_callback;
-	pcbstate-errcontext.arg = (void *) pcbstate;
-	pcbstate-errcontext.previous = error_context_stack;
-	error_context_stack = pcbstate-errcontext;
+	pcbstate-errcallback.callback = pcb_error_callback;
+	pcbstate-errcallback.arg = (void *) pcbstate;
+	pcbstate-errcallback.previous = error_context_stack;
+	error_context_stack = pcbstate-errcallback;
 }
 
 /*
@@ -157,7 +157,7 @@ void
 cancel_parser_errposition_callback(ParseCallbackState *pcbstate)
 {
 	/* Pop the error context stack */
-	error_context_stack = pcbstate-errcontext.previous;
+	error_context_stack = pcbstate-errcallback.previous;
 }
 
 /*
diff --git 

Re: [HACKERS] Different gettext domain needed for error context

[Heikki Linnakangas]

On 15.02.2012 20:13, Tom Lane wrote:

Heikki Linnakangasheikki.linnakan...@enterprisedb.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
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 0c301b2..e267ff2 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9913,6 +9913,9 @@ rm_redo_error_callback(void *arg)
 	XLogRecord *record = (XLogRecord *) arg;
 	StringInfoData buf;
 
+	/* use the right domain to translate the errcontext() calls */
+	set_errtextdomain();
+
 	initStringInfo(buf);
 	RmgrTable[record-xl_rmid].rm_desc(buf,
 	   record-xl_info,
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 1fffe1c..59ff1c9 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -914,6 +914,9 @@ sql_function_parse_error_callback(void *arg)
 {
 	parse_error_callback_arg *callback_arg = (parse_error_callback_arg *) arg;
 
+	/* use the right domain to translate the errcontext() calls */
+	set_errtextdomain();
+
 	/* See if it's a syntax error; if so, transpose to CREATE FUNCTION */
 	if (!function_parse_error_transpose(callback_arg-prosrc))
 	{
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 95fec8d..8533914 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1743,6 +1743,9 @@ CopyFromErrorCallback(void *arg)
 {
 	CopyState	cstate = (CopyState) arg;
 
+	/* use the right domain to translate the errcontext() calls */
+	set_errtextdomain();
+
 	if (cstate-binary)
 	{
 		/* can't usefully display the data */
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index ae8d374..7643733 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -1294,6 +1294,9 @@ sql_exec_error_callback(void *arg)
 	if (fcache == NULL || fcache-fname == NULL)
 		return;
 
+	/* use the right domain to translate the errcontext() calls */
+	set_errtextdomain();
+
 	/*
 	 * If there is a syntax error position, convert to internal syntax error
 	 */
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 5e4ae42..fd43106 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2154,7 +2154,10 @@ _SPI_error_callback(void 

Re: [HACKERS] Different gettext domain needed for error context

[Tom Lane]

Heikki Linnakangas heikki.linnakan...@enterprisedb.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Different gettext domain needed for error context

[Heikki Linnakangas]

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Different gettext domain needed for error context

[Tom Lane]

Heikki Linnakangas heikki.linnakan...@enterprisedb.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers