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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> > IIUC, the idea here is to check for parallel safety of functions at
> > someplace in the code during function invocation so that if we execute
> > any parallel unsafe/restricted function via parallel worker then we
> > error out. If so, isn't it possible to deal with built-in and
> > non-built-in functions in the same way?
>
> Yeah, one of the reasons I doubt this is a great idea is that you'd
> still have to fetch the pg_proc row for non-built-in functions.
>
> The obvious place to install such a check is fmgr_info(), which is
> fetching said row anyway for other purposes, so it's really hard to
> see how adding anything to FmgrBuiltin is going to help.

Thank you, fmgr_info() looks like the best place to do the parallel safety check. Having a quick look at its callers, I didn't find any concerning place (of course, we can't be relieved until the regression test succeeds.) Also, with fmgr_info(), we don't have to find other places to add the check to deal with functions calls in execExpr.c and execExprInterp.c. This is beautiful.

But the current fmgr_info() does not check the parallel safety of builtin functions. It does not have information to do that. There are two options. Which do you think is better? I think 2.

1) fmgr_info() reads pg_proc like for non-builtin functions
This ruins the effort for the fast path for builtin functions. I can't imagine how large the adverse impact on performance would be, but I'm worried.

The benefit is that ALTER FUNCTION on builtin functions takes effect. But such operations are nonsensical, so I don't think we want to gain such a benefit.

2) Gen_fmgrtab.pl adds a member for proparallel in FmgrBuiltin
But we don't want to enlarge FmgrBuiltin struct. So, change the existing bool members strict and and retset into one member of type char, and represent the original values with some bit flags. Then we use that member for proparallel as well. (As a result, one byte is left for future use.)

I think we'll try 2). I'd be grateful if you could point out anything I need to be careful to.

Regards
Takayuki Tsunakawa