Re: Error-safe user functions

On 2022-12-24 Sa 04:51, Ted Yu wrote:
>
>
> On Fri, Dec 23, 2022 at 1:22 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Ted Yu <yuzhihong(at)gmail(dot)com> writes:
> > In makeItemLikeRegex :
>
> > +                       /* See regexp.c for explanation */
> > +                       CHECK_FOR_INTERRUPTS();
> > +                       pg_regerror(re_result, &re_tmp, errMsg,
> > sizeof(errMsg));
> > +                       ereturn(escontext, false,
>
> > Since an error is returned, I wonder if the
> `CHECK_FOR_INTERRUPTS` call is
> > still necessary.
>
> Yes, it is.  We don't want a query-cancel transformed into a soft
> error.
>
>                         regards, tom lane
>
> Hi,
> For this case (`invalid regular expression`), the potential user
> interruption is one reason for stopping execution.
> I feel surfacing user interruption somehow masks the underlying error.
>
> The same regex, without user interruption, would exhibit an `invalid
> regular expression` error.
> I think it would be better to surface the error.
>
>

All that this patch is doing is replacing a call to
RE_compile_and_cache, which calls CHECK_FOR_INTERRUPTS, with similar
code, which gives us the opportunity to call ereturn instead of ereport.
Note that where escontext is NULL (the common case), ereturn functions
identically to ereport. So unless you want to argue that the logic in
RE_compile_and_cache is wrong I don't see what we're arguing about. If
instead I had altered the API of RE_compile_and_cache to include an
escontext parameter we wouldn't be having this argument at all. The only
reason I didn't do that was the point Tom quite properly raised about
why we're doing any caching here anyway.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com