Re: jsonpath

On 2019-01-29 04:17:33 +0300, Alexander Korotkov wrote:
> On Tue, Jan 29, 2019 at 4:03 AM Andres Freund <andres(at)anarazel(dot)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:

I find these *more* than sufficient to not go to the PG_TRY/CATCH
approach.

> 1) One of functions called here performs database modification, while
> it wasn't suppose to. So, it becomes not safe to skip subtransaction.

It's not just data modifications. Even just modifying some memory
structures that'd normally be invalidated by an xact abort's
invalidation processing isn't safe.

> 2) ERRCODE_DATA_EXCEPTION was thrown for unexpected reason. So, it
> might appear that ERRCODE_DATA_EXCEPTION is not safe to ignore.

It'd e.g. not surprise me very much if some OOM would end up translating
to ERRCODE_DATA_EXCEPTION, because some library function returned an
error due to ENOMEM.

> Could you complete this list?

3) The expression changed the current expression context, GUCs or any
other such global variable. Without a proper subtrans reset this
state isn't reverted.
4) The function acquires an LWLOCK, buffer reference, anything resowner
owned. Skipping subtrans reset, that's not released in that
moment. That's going to lead to potential hard deadlocks.
99) sigsetjmp is actually pretty expensive.

Greetings,

Andres Freund