Re: PG_DIAG_SEVERITY and a possible bug in pq_parse_errornotice()

On Fri, Aug 26, 2016 at 10:35 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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