On 2022-12-24 Sa 04:51, Ted Yu wrote: > > > On Fri, Dec 23, 2022 at 1:22 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Ted Yu <yuzhihong@gmail.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.