From: | "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Greg Nancarrow <gregn4422(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "tsunakawa(dot)takay(at)fujitsu(dot)com" <tsunakawa(dot)takay(at)fujitsu(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | RE: [bug?] Missed parallel safety checks, and wrong parallel safety |
Date: | 2021-05-11 06:58:17 |
Message-ID: | OS0PR01MB571646637784DAF1DD4C8BE994539@OS0PR01MB5716.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
> > Sometimes this is actually quite useful. You might know that, while
> > the function is in general volatile, it is immutable in the particular
> > way that you are using it. Or, perhaps, you are using the volatile
> > function incidentally and it doesn't affect the output of your
> > function at all. Or, maybe you actually want to build an index that
> > might break, and then it's up to you to rebuild the index if and when
> > that is required. Users do this kind of thing all the time, I think,
> > and would be unhappy if we started checking it more rigorously than we
> > do today.
> >
> > Now, I don't see why the same idea can't or shouldn't apply to
> > parallel-safety. If you call a parallel-unsafe function in a parallel
> > context, it's pretty likely that you are going to get an error, and so
> > you might not want to do it. If the function is written in C, it could
> > even cause horrible things to happen so that you crash the whole
> > backend or something, but I tried to set things up so that for
> > built-in functions you'll just get an error. But on the other hand,
> > maybe the parallel-unsafe function you are calling is not
> > parallel-unsafe in all cases. If you want to create a wrapper function
> > that is labelled parallel-safe and try to make that it only calls the
> > parallel-unsafe function in the cases where there's no safety problem,
> > that's up to you!
> >
>
> I think it is difficult to say for what purpose parallel-unsafe function got called in
> parallel context so if we give an error in cases where otherwise it could lead to
> a crash or caused other horrible things, users will probably appreciate us.
> OTOH, if the parallel-safety labeling is wrong (parallel-safe function is marked
> parallel-unsafe) and we gave an error in such a case, the user can always change
> the parallel-safety attribute by using Alter Function.
> Now, if adding such a check is costly or needs some major re-design then
> probably it might not be worth whereas I don't think that is the case for
> non-built-in function invocation.
Temporarily, Just in case someone want to take a look at the patch for the safety check.
I splited the patch into 0001(parallel safety check for user define function), 0003(parallel safety check for builtin function)
and the fix for testcases.
IMO, With such a check to give an error when detecting parallel unsafe function in parallel mode,
it will be easier for users to discover potential threats(parallel unsafe function) in parallel mode.
I think users is likely to invoke parallel unsafe function inner a parallel safe function unintentionally.
Such a check can help they detect the problem easier.
Although, the strict check limits some usages(intentionally wrapper function) like Robert-san said.
To mitigate the effect of the limit, I was thinking can we do the safety check conditionally, such as only check the top function invoke and/or
introduce a guc option to control whether do the strict parallel safety check? Thoughts ?
Best regards,
houzj
Attachment | Content-Type | Size |
---|---|---|
0003-check-built-in-function-parallel-safety-in-fmgr_info.patch | application/octet-stream | 4.3 KB |
0004-fix-builtin-parallel-safety-label-in-testcase.patch | application/octet-stream | 2.1 KB |
0001-check-UDF-parallel-safety-in-fmgr_info.patch | application/octet-stream | 2.3 KB |
0002-fix-UDF-parallel-safety-label-in-testcase.patch | application/octet-stream | 8.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Andrey Lepikhov | 2021-05-11 07:06:05 | Re: Defer selection of asynchronous subplans until the executor initialization stage |
Previous Message | Fujii Masao | 2021-05-11 06:34:18 | Re: compute_query_id and pg_stat_statements |