Re: [bug?] Missed parallel safety checks, and wrong parallel safety

On Wed, Apr 28, 2021 at 9:42 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> So, If we do not want to lock down the parallel safety of built-in functions.
> It seems we can try to fetch the proparallel from pg_proc for built-in function
> in fmgr_info_cxt_security too. To avoid recursive safety check when fetching
> proparallel from pg_proc, we can add a Global variable to mark is it in a recursive state.
> And we skip safety check in a recursive state, In this approach, parallel safety
> will not be locked, and there are no new members in FmgrBuiltin.
>
> Attaching the patch about this approach [0001-approach-1].
> Thoughts ?

This seems to be full of complicated if-tests that don't seem
necessary and aren't explained by the comments. Also, introducing a
system cache lookup here seems completely unacceptable from a
reliability point of view, and I bet it's not too good for
performance, either.

> I also attached another approach patch [0001-approach-2] about adding
> parallel safety in FmgrBuiltin, because this approach seems faster and
> we can combine some bool member into a bitflag to avoid enlarging the
> FmgrBuiltin array, though this approach will lock down the parallel safety
> of built-in function.

This doesn't seem like a good idea either.

I really don't understand what problem any of this is intended to
solve. Bharath's analysis above seems right on point to me. I think if
anybody is writing a patch that requires that this be changed in this
way, that person is probably doing something wrong.

--
Robert Haas
EDB: http://www.enterprisedb.com