On Tue, Jan 29, 2019 at 4:03 AM Andres Freund <andres@anarazel.de> wrote:
> On 2019-01-29 04:00:19 +0300, Alexander Korotkov wrote:
> > + /*
> > + * It is safe to use here PG_TRY/PG_CATCH without subtransaction because
> > + * no function called inside performs data modification.
> > + */
> > + PG_TRY();
> > + {
> > + res = DirectFunctionCall2(func, ldatum, rdatum);
> > + }
> > + PG_CATCH();
> > + {
> > + int errcode = geterrcode();
> > +
> > + if (jspThrowErrors(cxt) ||
> > + ERRCODE_TO_CATEGORY(errcode) != ERRCODE_DATA_EXCEPTION)
> > + PG_RE_THROW();
> > +
> > + MemoryContextSwitchTo(mcxt);
> > + FlushErrorState();
> > +
> > + return jperError;
> > + }
> > + PG_END_TRY();
>
> FWIW, I still think this is a terrible idea and shouldn't be merged this
> way. The likelihood of introducing subtle bugs seems way too high - even
> if it's possibly not buggy today, who says that it's not going to be in
> the future?
I'm probably not yet understanding all the risks this code have. So far I see:
1) One of functions called here performs database modification, while
it wasn't suppose to. So, it becomes not safe to skip subtransaction.
2) ERRCODE_DATA_EXCEPTION was thrown for unexpected reason. So, it
might appear that ERRCODE_DATA_EXCEPTION is not safe to ignore.
Could you complete this list?
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
