Alexander Korotkov <a.korot...@postgrespro.ru> writes:
> On Tue, Jan 29, 2019 at 4:03 AM Andres Freund <and...@anarazel.de> wrote:
>> 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?
Sure: every errcode we have is unsafe to treat this way.
The backend coding rule from day one has been that a thrown error requires
(sub)transaction cleanup to be done to make sure that things are back in a
good state. You can *not* just decide that it's okay to ignore that,
especially not when invoking code outside the immediate area of what
you're doing.
As a counterexample, for any specific errcode you might claim is safe,
a plpgsql function might do something that requires cleanup (which is
not equal to "does data modification") and then throw that errcode.
You might argue that this code will only ever be used to call certain
functions in numeric.c and you've vetted every line of code that those
functions can reach ... but even to say that is to expose how fragile
the argument is. Every time anybody touched numeric.c, or any code
reachable from there, we'd have to wonder whether we just introduced
a failure hazard for jsonpath. That's not maintainable. It's even
less maintainable if anybody decides they'd like to insert extension
hooks into any of that code. I rather imagine that this could be
broken today by an ErrorContextCallback function, for example.
(That is, even today, "vetting every line" has to include vetting every
errcontext subroutine in the system, along with everything it can call.
Plus equivalent code in external PLs and so on.)
We've rejected code like this every time it was submitted to date,
and I don't see a reason to change that policy for jsonpath.
regards, tom lane
