Re: PG_DIAG_SEVERITY and a possible bug in pq_parse_errornotice()

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