On Sat, Dec 24, 2022 at 4:38 AM Andrew Dunstan <and...@dunslane.net> wrote:
> > On 2022-12-24 Sa 04:51, Ted Yu wrote: > > > > > > On Fri, Dec 23, 2022 at 1:22 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > > > > Ted Yu <yuzhih...@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. > > > cheers > > > andrew > > -- > Andrew Dunstan > EDB: https://www.enterprisedb.com Andrew: Thanks for the response.
