Re: PG_DIAG_SEVERITY and a possible bug in pq_parse_errornotice()

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Aug 26, 2016 at 10:35 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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
be
oldcontext = 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