Thread: PG_DIAG_SEVERITY and a possible bug in pq_parse_errornotice()
Hi, My PostgreSQL client checks the PG_DIAG_SEVERITY error field to determine the error level. However, I have now learned that this field is localized. This means that a server configured with --enable-nls might forexample return the string ERREUR instead of ERROR. So if I want to determine the error level, do I need to compare against all localised variations in my app? Or is there anotherway? I browsed through the PostgreSQL source and discovered that pq_parse_errornotice() (in src/backend/libpq/pqmq.c) uses thesame naive strcmp() approach to determine error level. This means that it will fail when the server is compiled with --enable-nls.I am not sure what the impact of this is, since I couldn't really figure out where that function is used. Best regards, Jakob
Jakob Egger <jakob@eggerapps.at> writes:
> My PostgreSQL client checks the PG_DIAG_SEVERITY error field to determine the error level.
> However, I have now learned that this field is localized. This means that a server configured with --enable-nls might
forexample return the string ERREUR instead of ERROR.
Check.
> So if I want to determine the error level, do I need to compare against all localised variations in my app? Or is
thereanother way?
Generally, we've presumed that clients don't really need to know the
difference between error levels, beyond the error-versus-notice
distinction that's embedded in the message type. If you have an
application where that's actually important, it would be interesting to
know what it is.
> I browsed through the PostgreSQL source and discovered that pq_parse_errornotice() (in src/backend/libpq/pqmq.c) uses
thesame naive strcmp() approach to determine error level. This means that it will fail when the server is compiled with
--enable-nls.I am not sure what the impact of this is, since I couldn't really figure out where that function is used.
Ooops. Indeed, that is broken:
postgres=# select 1/0; -- using French locale
ERREUR: division par zéro
postgres=# set force_parallel_mode=1;
SET
postgres=# select stringu1::int2 from tenk1 where unique1 = 1;
ERREUR: unknown error severity
CONTEXT: parallel worker
Not sure what we ought to do about that, but we need to do something.
regards, tom lane
On Thu, Aug 25, 2016 at 11:43 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Ooops. Indeed, that is broken: > > postgres=# select 1/0; -- using French locale > ERREUR: division par zéro > postgres=# set force_parallel_mode=1; > SET > postgres=# select stringu1::int2 from tenk1 where unique1 = 1; > ERREUR: unknown error severity > CONTEXT: parallel worker > > Not sure what we ought to do about that, but we need to do something. Uggh. Obviously, I failed to realize that those strings were localized. Leaving aside the question of this particular matching problem, I wonder if we are localizing everything twice right now, once in the worker and once in the leader. It's probably best to try to hack things somehow so that the worker localizes nothing and the leader localizes everything. Or we could add another field to the message the worker sends that includes the error level as an integer. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Aug 25, 2016 at 11:43 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Ooops. Indeed, that is broken:
>> postgres=# select stringu1::int2 from tenk1 where unique1 = 1;
>> ERREUR: unknown error severity
>> CONTEXT: parallel worker
> Uggh. Obviously, I failed to realize that those strings were
> localized. Leaving aside the question of this particular matching
> problem, I wonder if we are localizing everything twice right now,
> once in the worker and once in the leader.
Hm. It's possible we're passing already-localized strings through
gettext a second time in the leader, but that basically does nothing
(unless somehow you got a match, but the probability seems tiny).
I rather doubt that is happening though, because of the next point:
> It's probably best to try
> to hack things somehow so that the worker localizes nothing and the
> leader localizes everything.
No way that's gonna work. For example, the expected report in
English for the example above isERROR: invalid input syntax for integer: "BAAAAA"
That doesn't match anything in gettext's database, because we
already substituted something for the %s. Basically, localization
always has to happen before error message construction, not later.
> Or we could add another field to the
> message the worker sends that includes the error level as an integer.
The two alternatives that seem reasonable to me after a bit of reflection
are:
1. Hack elog.c to not localize the severity field when in a worker
process. (It'd still have to localize all the other fields, because
of the %s problem.) This would be a very localized fix, but it still
seems mighty ugly.
2. Add another field to error messages, which would be a never-localized
copy of the severity string. This might be the best long-run solution,
especially if Jakob can present a convincing argument why clients might
want it. We would be taking some small chance of breaking existing
clients, but if it only happens in a new major release (ie 9.6) then
that doesn't seem like a problem. And anyway the protocol spec has
always counseled clients to be prepared to ignore unrecognized fields
in an error message.
We could do a variant 2a in which we invent an additional field but
only allow workers to send it, which is more or less the same as your
idea (though I'd still prefer string to integer). I don't find that
very attractive though. If we're going to hack elog.c to have different
sending behavior in a worker, we might as well do #1.
regards, tom lane
On Thu, Aug 25, 2016 at 1:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> It's probably best to try >> to hack things somehow so that the worker localizes nothing and the >> leader localizes everything. > > No way that's gonna work. For example, the expected report in > English for the example above is > ERROR: invalid input syntax for integer: "BAAAAA" > That doesn't match anything in gettext's database, because we > already substituted something for the %s. Basically, localization > always has to happen before error message construction, not later. Oh, right. >> Or we could add another field to the >> message the worker sends that includes the error level as an integer. > > The two alternatives that seem reasonable to me after a bit of reflection > are: > > 1. Hack elog.c to not localize the severity field when in a worker > process. (It'd still have to localize all the other fields, because > of the %s problem.) This would be a very localized fix, but it still > seems mighty ugly. > > 2. Add another field to error messages, which would be a never-localized > copy of the severity string. This might be the best long-run solution, > especially if Jakob can present a convincing argument why clients might > want it. We would be taking some small chance of breaking existing > clients, but if it only happens in a new major release (ie 9.6) then > that doesn't seem like a problem. And anyway the protocol spec has > always counseled clients to be prepared to ignore unrecognized fields > in an error message. > > We could do a variant 2a in which we invent an additional field but > only allow workers to send it, which is more or less the same as your > idea (though I'd still prefer string to integer). I don't find that > very attractive though. If we're going to hack elog.c to have different > sending behavior in a worker, we might as well do #1. I don't have strong feelings about this. Technically, this issue affects 9.5 also, because pqmq.c was introduced in that release. I don't think we want to add another error field in a released branch. However, since there's no parallel query in 9.5, only people who are accessing that functionality via extension code would be affected, which might be nobody and certainly isn't a lot of people, so we could just leave this unfixed in 9.5. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
BTW, this example also exposes another problem, in that the report is
>> ERREUR: unknown error severity
>> CONTEXT: parallel worker
Since, in fact, this error was thrown from leader code, that CONTEXT
report is outright misleading. It's easy to figure that out in this
particular case because the error could only have come from the leader
side, but errors reported from further down inside pq_parse_errornotice
(eg, out-of-memory in pstrdup) would not be so easy.
We could reduce this problem by narrowing the scope over which
HandleParallelMessage activates the ParallelErrorContext callback.
But I'm inclined to get rid of that callback entirely and instead
do something like this:
/* Parse ErrorResponse or NoticeResponse. */ pq_parse_errornotice(msg, &edata);
/* Death of a worker isn't enough justification for suicide. */ edata.elevel =
Min(edata.elevel,ERROR);
+ /* If desired, tag the message with context. */
+ if (force_parallel_mode != FORCE_PARALLEL_REGRESS)
+ {
+ if (edata.context)
+ edata.context = psprintf("%s\n%s", edata.context,
+ _("parallel worker"));
+ else
+ edata.context = pstrdup(_("parallel worker"));
+ }
+ /* Rethrow error or notice. */ ThrowErrorData(&edata);
This knows a little bit more than before about the conventions for
combining context strings, but we can be sure that "parallel worker"
will be appended to exactly the messages that come back from the
parallel worker, not anything else.
Barring objections I'll push something like this tomorrow.
regards, tom lane
Robert Haas <robertmhaas@gmail.com> writes:
> I don't have strong feelings about this. Technically, this issue
> affects 9.5 also, because pqmq.c was introduced in that release. I
> don't think we want to add another error field in a released branch.
> However, since there's no parallel query in 9.5, only people who are
> accessing that functionality via extension code would be affected,
> which might be nobody and certainly isn't a lot of people, so we could
> just leave this unfixed in 9.5.
After sleeping on it, I think the right answer is to introduce the new
error-message field (and not worry about 9.5). Will work on a patch
for that, unless I hear objections pretty soon.
regards, tom lane
I wrote:
> After sleeping on it, I think the right answer is to introduce the new
> error-message field (and not worry about 9.5). Will work on a patch
> for that, unless I hear objections pretty soon.
BTW, while I'm looking at this: what on god's green earth is
ThrowErrorData doing copying the supplied data into edata->assoc_context?
Surely it should be putting the data into the ErrorContext, where it's not
going to get flushed before it can be reported? (Note that in the sole
existing use-case, edata->assoc_context is going to have been set to
CurrentMemoryContext by pq_parse_errornotice, and I see no good reason to
assume that's very long-lived ... in fact, it looks like it's whatever
happens to be active when ProcessInterrupts is called, which means there's
probably a totally separate set of problems here having to do with
potential leaks into long-lived contexts.)
I have little use for the name of that function either, as it's not
necessarily going to "throw" anything. Maybe ReportErrorUsingData,
or something like that?
regards, tom lane
On Fri, Aug 26, 2016 at 10:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> After sleeping on it, I think the right answer is to introduce the new
>> error-message field (and not worry about 9.5). Will work on a patch
>> for that, unless I hear objections pretty soon.
>
> BTW, while I'm looking at this: what on god's green earth is
> ThrowErrorData doing copying the supplied data into edata->assoc_context?
> Surely it should be putting the data into the ErrorContext, where it's not
> going to get flushed before it can be reported?
Uh, well, perhaps I misinterpreted the comment in elog.h. It says this:
/* context containing associated non-constant strings */ struct MemoryContextData *assoc_context;
That sure looks like it's saying that all of the pointers stored in
the ErrorData structure should be pointing into assoc_context, unless
they are constant. If that's not right, I suggest rewording that
comment, because I cannot think of a second interpretation of what's
written there.
> (Note that in the sole
> existing use-case, edata->assoc_context is going to have been set to
> CurrentMemoryContext by pq_parse_errornotice, and I see no good reason to
> assume that's very long-lived ... in fact, it looks like it's whatever
> happens to be active when ProcessInterrupts is called, which means there's
> probably a totally separate set of problems here having to do with
> potential leaks into long-lived contexts.)
Oops. Yes.
> I have little use for the name of that function either, as it's not
> necessarily going to "throw" anything. Maybe ReportErrorUsingData,
> or something like that?
I deliberately avoided that sort of terminology because it need not be
an ERROR. It can be, say, a NOTICE. It is definitely something that
is coming from an ErrorData but it need not be an ERROR.
Also, I think "throwing an error" is pretty standard terminology that
is understandable to pretty much all programmers these days. It's not
a perfect name; maybe ReportErrorData would have been better, but
changing that seems like pointless tinkering at this stage, from my
point of view.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Aug 26, 2016 at 10:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> BTW, while I'm looking at this: what on god's green earth is
>> ThrowErrorData doing copying the supplied data into edata->assoc_context?
>> Surely it should be putting the data into the ErrorContext, where it's not
>> going to get flushed before it can be reported?
> Uh, well, perhaps I misinterpreted the comment in elog.h. It says this:
> /* context containing associated non-constant strings */
> struct MemoryContextData *assoc_context;
> That sure looks like it's saying that all of the pointers stored in
> the ErrorData structure should be pointing into assoc_context, unless
> they are constant.
Indeed. The point is that every ErrorData in the errordata stack needs
to, and does, have assoc_context = ErrorContext. This coding is blithely
ignoring what errstart has set up and copying the data someplace else.
In fact, it's pointlessly duplicating data that is *already* in the
context of the source ErrorData.
You should be imagining this action as being the reverse of CopyErrorData,
ie copying some data back into the purview of elog.c, which is to say it
should be in ErrorContext.
Or in short, this has confused edata and newedata. Valid coding would
beoldcontext = MemoryContextSwitchTo(newedata->assoc_context);
rather than what is there.
>> (Note that in the sole
>> existing use-case, edata->assoc_context is going to have been set to
>> CurrentMemoryContext by pq_parse_errornotice, and I see no good reason to
>> assume that's very long-lived ... in fact, it looks like it's whatever
>> happens to be active when ProcessInterrupts is called, which means there's
>> probably a totally separate set of problems here having to do with
>> potential leaks into long-lived contexts.)
> Oops. Yes.
I'm not quite sure what to do about this. It feels a tad wrong to use
ErrorContext as the active context during HandleParallelMessages, but
what else should we use? Maybe this needs its very own private context
that we can reset after each use?
>> I have little use for the name of that function either, as it's not
>> necessarily going to "throw" anything. Maybe ReportErrorUsingData,
>> or something like that?
> I deliberately avoided that sort of terminology because it need not be
> an ERROR. It can be, say, a NOTICE. It is definitely something that
> is coming from an ErrorData but it need not be an ERROR.
Right, but to me, "throw" generally means a transfer of control, which
is exactly what this isn't going to do if the message is only a notice.
regards, tom lane
I wrote:
> After sleeping on it, I think the right answer is to introduce the new
> error-message field (and not worry about 9.5). Will work on a patch
> for that, unless I hear objections pretty soon.
So far as I can find, the attached is all we need to do to introduce a
new message field. (This patch doesn't address the memory-context
questions, but it does fix the localization-driven failure demonstrated
upthread.)
Any objections? Anyone want to bikeshed the field name? I considered
PG_DIAG_SEVERITY_NONLOCALIZED and PG_DIAG_SEVERITY_ENGLISH before settling
on PG_DIAG_SEVERITY_ASCII, but I can't say I'm in love with that.
regards, tom lane
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index f22e3da..40ae0ff 100644
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
*************** char *PQresultErrorField(const PGresult
*** 2767,2772 ****
--- 2767,2788 ----
</listitem>
</varlistentry>
+ <varlistentry id="libpq-pg-diag-severity-ascii">
+ <term><symbol>PG_DIAG_SEVERITY_ASCII</></term>
+ <listitem>
+ <para>
+ The severity; the field contents are <literal>ERROR</>,
+ <literal>FATAL</>, or <literal>PANIC</> (in an error message),
+ or <literal>WARNING</>, <literal>NOTICE</>, <literal>DEBUG</>,
+ <literal>INFO</>, or <literal>LOG</> (in a notice message).
+ This is identical to the <symbol>PG_DIAG_SEVERITY</> field except
+ that the contents are never localized. This is present only in
+ reports generated by <productname>PostgreSQL</> versions 9.6
+ and later.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="libpq-pg-diag-sqlstate">
<term>
<symbol>PG_DIAG_SQLSTATE</>
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 9c96d8f..9bddb19 100644
*** a/doc/src/sgml/protocol.sgml
--- b/doc/src/sgml/protocol.sgml
*************** message.
*** 4884,4889 ****
--- 4884,4908 ----
<varlistentry>
<term>
+ <literal>A</>
+ </term>
+ <listitem>
+ <para>
+ Severity: the field contents are
+ <literal>ERROR</>, <literal>FATAL</>, or
+ <literal>PANIC</> (in an error message), or
+ <literal>WARNING</>, <literal>NOTICE</>, <literal>DEBUG</>,
+ <literal>INFO</>, or <literal>LOG</> (in a notice message).
+ This is identical to the <literal>S</> field except
+ that the contents are never localized. This is present only in
+ messages generated by <productname>PostgreSQL</> versions 9.6
+ and later.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term>
<literal>C</>
</term>
<listitem>
diff --git a/src/backend/libpq/pqmq.c b/src/backend/libpq/pqmq.c
index 921242f..b04a7ef 100644
*** a/src/backend/libpq/pqmq.c
--- b/src/backend/libpq/pqmq.c
*************** pq_parse_errornotice(StringInfo msg, Err
*** 237,246 ****
switch (code)
{
case PG_DIAG_SEVERITY:
if (strcmp(value, "DEBUG") == 0)
! edata->elevel = DEBUG1; /* or some other DEBUG level */
else if (strcmp(value, "LOG") == 0)
! edata->elevel = LOG; /* can't be COMMERROR */
else if (strcmp(value, "INFO") == 0)
edata->elevel = INFO;
else if (strcmp(value, "NOTICE") == 0)
--- 237,262 ----
switch (code)
{
case PG_DIAG_SEVERITY:
+ /* ignore, trusting we'll get a nonlocalized version */
+ break;
+ case PG_DIAG_SEVERITY_ASCII:
if (strcmp(value, "DEBUG") == 0)
! {
! /*
! * We can't reconstruct the exact DEBUG level, but
! * presumably it was >= client_min_messages, so select
! * DEBUG1 to ensure we'll pass it on to the client.
! */
! edata->elevel = DEBUG1;
! }
else if (strcmp(value, "LOG") == 0)
! {
! /*
! * It can't be LOG_SERVER_ONLY, or the worker wouldn't
! * have sent it to us; so LOG is the correct value.
! */
! edata->elevel = LOG;
! }
else if (strcmp(value, "INFO") == 0)
edata->elevel = INFO;
else if (strcmp(value, "NOTICE") == 0)
*************** pq_parse_errornotice(StringInfo msg, Err
*** 254,264 ****
else if (strcmp(value, "PANIC") == 0)
edata->elevel = PANIC;
else
! elog(ERROR, "unknown error severity");
break;
case PG_DIAG_SQLSTATE:
if (strlen(value) != 5)
! elog(ERROR, "malformed sql state");
edata->sqlerrcode = MAKE_SQLSTATE(value[0], value[1], value[2],
value[3], value[4]);
break;
--- 270,280 ----
else if (strcmp(value, "PANIC") == 0)
edata->elevel = PANIC;
else
! elog(ERROR, "unrecognized error severity: \"%s\"", value);
break;
case PG_DIAG_SQLSTATE:
if (strlen(value) != 5)
! elog(ERROR, "invalid SQLSTATE: \"%s\"", value);
edata->sqlerrcode = MAKE_SQLSTATE(value[0], value[1], value[2],
value[3], value[4]);
break;
*************** pq_parse_errornotice(StringInfo msg, Err
*** 308,314 ****
edata->funcname = pstrdup(value);
break;
default:
! elog(ERROR, "unknown error field: %d", (int) code);
break;
}
}
--- 324,330 ----
edata->funcname = pstrdup(value);
break;
default:
! elog(ERROR, "unrecognized error field code: %d", (int) code);
break;
}
}
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 78d441d..ab71b50 100644
*** a/src/backend/utils/error/elog.c
--- b/src/backend/utils/error/elog.c
*************** write_csvlog(ErrorData *edata)
*** 2744,2750 ****
appendStringInfoChar(&buf, ',');
/* Error severity */
! appendStringInfoString(&buf, error_severity(edata->elevel));
appendStringInfoChar(&buf, ',');
/* SQL state code */
--- 2744,2750 ----
appendStringInfoChar(&buf, ',');
/* Error severity */
! appendStringInfoString(&buf, gettext(error_severity(edata->elevel)));
appendStringInfoChar(&buf, ',');
/* SQL state code */
*************** send_message_to_server_log(ErrorData *ed
*** 2861,2867 ****
formatted_log_time[0] = '\0';
log_line_prefix(&buf, edata);
! appendStringInfo(&buf, "%s: ", error_severity(edata->elevel));
if (Log_error_verbosity >= PGERROR_VERBOSE)
appendStringInfo(&buf, "%s: ", unpack_sql_state(edata->sqlerrcode));
--- 2861,2867 ----
formatted_log_time[0] = '\0';
log_line_prefix(&buf, edata);
! appendStringInfo(&buf, "%s: ", gettext(error_severity(edata->elevel)));
if (Log_error_verbosity >= PGERROR_VERBOSE)
appendStringInfo(&buf, "%s: ", unpack_sql_state(edata->sqlerrcode));
*************** send_message_to_frontend(ErrorData *edat
*** 3144,3155 ****
if (PG_PROTOCOL_MAJOR(FrontendProtocol) >= 3)
{
/* New style with separate fields */
char tbuf[12];
int ssval;
int i;
pq_sendbyte(&msgbuf, PG_DIAG_SEVERITY);
! err_sendstring(&msgbuf, error_severity(edata->elevel));
/* unpack MAKE_SQLSTATE code */
ssval = edata->sqlerrcode;
--- 3144,3159 ----
if (PG_PROTOCOL_MAJOR(FrontendProtocol) >= 3)
{
/* New style with separate fields */
+ const char *sev;
char tbuf[12];
int ssval;
int i;
+ sev = error_severity(edata->elevel);
pq_sendbyte(&msgbuf, PG_DIAG_SEVERITY);
! err_sendstring(&msgbuf, gettext(sev));
! pq_sendbyte(&msgbuf, PG_DIAG_SEVERITY_ASCII);
! err_sendstring(&msgbuf, sev);
/* unpack MAKE_SQLSTATE code */
ssval = edata->sqlerrcode;
*************** send_message_to_frontend(ErrorData *edat
*** 3268,3274 ****
initStringInfo(&buf);
! appendStringInfo(&buf, "%s: ", error_severity(edata->elevel));
if (edata->show_funcname && edata->funcname)
appendStringInfo(&buf, "%s: ", edata->funcname);
--- 3272,3278 ----
initStringInfo(&buf);
! appendStringInfo(&buf, "%s: ", gettext(error_severity(edata->elevel)));
if (edata->show_funcname && edata->funcname)
appendStringInfo(&buf, "%s: ", edata->funcname);
*************** get_errno_symbol(int errnum)
*** 3578,3584 ****
/*
! * error_severity --- get localized string representing elevel
*/
static const char *
error_severity(int elevel)
--- 3582,3591 ----
/*
! * error_severity --- get string representing elevel
! *
! * The string is not localized here, but we mark the strings for translation
! * so that callers can invoke gettext on the result.
*/
static const char *
error_severity(int elevel)
*************** error_severity(int elevel)
*** 3592,3620 ****
case DEBUG3:
case DEBUG4:
case DEBUG5:
! prefix = _("DEBUG");
break;
case LOG:
case LOG_SERVER_ONLY:
! prefix = _("LOG");
break;
case INFO:
! prefix = _("INFO");
break;
case NOTICE:
! prefix = _("NOTICE");
break;
case WARNING:
! prefix = _("WARNING");
break;
case ERROR:
! prefix = _("ERROR");
break;
case FATAL:
! prefix = _("FATAL");
break;
case PANIC:
! prefix = _("PANIC");
break;
default:
prefix = "???";
--- 3599,3627 ----
case DEBUG3:
case DEBUG4:
case DEBUG5:
! prefix = gettext_noop("DEBUG");
break;
case LOG:
case LOG_SERVER_ONLY:
! prefix = gettext_noop("LOG");
break;
case INFO:
! prefix = gettext_noop("INFO");
break;
case NOTICE:
! prefix = gettext_noop("NOTICE");
break;
case WARNING:
! prefix = gettext_noop("WARNING");
break;
case ERROR:
! prefix = gettext_noop("ERROR");
break;
case FATAL:
! prefix = gettext_noop("FATAL");
break;
case PANIC:
! prefix = gettext_noop("PANIC");
break;
default:
prefix = "???";
diff --git a/src/include/postgres_ext.h b/src/include/postgres_ext.h
index 74c344c..e318e18 100644
*** a/src/include/postgres_ext.h
--- b/src/include/postgres_ext.h
*************** typedef PG_INT64_TYPE pg_int64;
*** 49,54 ****
--- 49,55 ----
* applications.
*/
#define PG_DIAG_SEVERITY 'S'
+ #define PG_DIAG_SEVERITY_ASCII 'A'
#define PG_DIAG_SQLSTATE 'C'
#define PG_DIAG_MESSAGE_PRIMARY 'M'
#define PG_DIAG_MESSAGE_DETAIL 'D'
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index d1b91c8..58c8cab 100644
*** a/src/interfaces/libpq/fe-exec.c
--- b/src/interfaces/libpq/fe-exec.c
*************** pqInternalNotice(const PGNoticeHooks *ho
*** 824,829 ****
--- 824,830 ----
*/
pqSaveMessageField(res, PG_DIAG_MESSAGE_PRIMARY, msgBuf);
pqSaveMessageField(res, PG_DIAG_SEVERITY, libpq_gettext("NOTICE"));
+ pqSaveMessageField(res, PG_DIAG_SEVERITY_ASCII, "NOTICE");
/* XXX should provide a SQLSTATE too? */
/*
Tom Lane wrote: > So far as I can find, the attached is all we need to do to introduce a > new message field. (This patch doesn't address the memory-context > questions, but it does fix the localization-driven failure demonstrated > upthread.) > > Any objections? Anyone want to bikeshed the field name? I considered > PG_DIAG_SEVERITY_NONLOCALIZED and PG_DIAG_SEVERITY_ENGLISH before settling > on PG_DIAG_SEVERITY_ASCII, but I can't say I'm in love with that. I didn't review the patch, but +1 on the idea. As for the name, I think ASCII is the wrong thing (as many labels in other languages can be in ascii too). I vote for NONLOCALIZED. I see character "s" is already taken in the protocol; that would be my first preference rather than A. How about Z? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> Any objections? Anyone want to bikeshed the field name? I considered
>> PG_DIAG_SEVERITY_NONLOCALIZED and PG_DIAG_SEVERITY_ENGLISH before settling
>> on PG_DIAG_SEVERITY_ASCII, but I can't say I'm in love with that.
> I didn't review the patch, but +1 on the idea. As for the name, I think
> ASCII is the wrong thing (as many labels in other languages can be in
> ascii too). I vote for NONLOCALIZED.
> I see character "s" is already taken in the protocol; that would be my
> first preference rather than A. How about Z?
One of the reasons I didn't pick NONLOCALIZED was that I couldn't come
up with an appropriate letter for the protocol. I guess Z is okay,
but it's a bit of a stretch (especially if you're of the persuasion
that wants to spell it "nonlocalised"). Maybe V for "seVerity"?
regards, tom lane
On Fri, Aug 26, 2016 at 11:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Or in short, this has confused edata and newedata. Valid coding would > be > oldcontext = MemoryContextSwitchTo(newedata->assoc_context); > rather than what is there. Oh, right. >>> (Note that in the sole >>> existing use-case, edata->assoc_context is going to have been set to >>> CurrentMemoryContext by pq_parse_errornotice, and I see no good reason to >>> assume that's very long-lived ... in fact, it looks like it's whatever >>> happens to be active when ProcessInterrupts is called, which means there's >>> probably a totally separate set of problems here having to do with >>> potential leaks into long-lived contexts.) > >> Oops. Yes. > > I'm not quite sure what to do about this. It feels a tad wrong to use > ErrorContext as the active context during HandleParallelMessages, but > what else should we use? Maybe this needs its very own private context > that we can reset after each use? If we use ErrorContext, will anything go wrong? >>> I have little use for the name of that function either, as it's not >>> necessarily going to "throw" anything. Maybe ReportErrorUsingData, >>> or something like that? > >> I deliberately avoided that sort of terminology because it need not be >> an ERROR. It can be, say, a NOTICE. It is definitely something that >> is coming from an ErrorData but it need not be an ERROR. > > Right, but to me, "throw" generally means a transfer of control, which > is exactly what this isn't going to do if the message is only a notice. Fair point. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Aug 26, 2016 at 11:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm not quite sure what to do about this. It feels a tad wrong to use
>> ErrorContext as the active context during HandleParallelMessages, but
>> what else should we use? Maybe this needs its very own private context
>> that we can reset after each use?
> If we use ErrorContext, will anything go wrong?
I think it might, if we were unlucky enough to be doing this while the
leader was engaged in reporting some other error for its own reasons.
To avoid accumulated memory leakage over multiple worker error reports,
we ought to do a MemoryContextReset at the start or end (probably start)
of HandleParallelMessages, and that would be problematic if ErrorContext
had some data in it already. It's possible that the scenario can't occur
because CHECK_FOR_INTERRUPTS is never done inside error processing, but
I would not feel very comfortable with that assumption.
I'm thinking a dedicated context is the way to go. It won't take very
much code.
regards, tom lane