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

On Wed, May 5, 2021 at 7:39 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Tue, May 4, 2021 at 11:47 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> > Problem is, for built-in functions, the changes are allowed, but for
> > some properties (like strict) the allowed changes don't actually take
> > effect (this is what Amit was referring to - so why allow those
> > changes?).
> > It's because some of the function properties are cached in
> > FmgrBuiltins[] (for a "fast-path" lookup for built-ins), according to
> > their state at build time (from pg_proc.dat), but ALTER FUNCTION is
> > just changing it in the system catalogs. Also, with sufficient
> > privileges, a built-in function can be redefined, yet the original
> > function (whose info is cached in FmgrBuiltins[]) is always invoked,
> > not the newly-defined version.
>
> I agree. I think that's not ideal. I think we should consider putting
> some more restrictions on updating system catalog changes, and I also
> think that if we can get out of having strict need to be part of
> FmgrBuiltins[] that would be good. But what I don't agree with is the
> idea that since strict already has this problem, it's OK to do the
> same thing with parallel-safety. That seems to me to be making a bad
> situation worse, and I can't see what problem it actually solves.
>

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. I think that is a good safety net especially if we can do
it with some simple check. Now, we already have pg_proc information in
fmgr_info_cxt_security for non-built-in functions, so we can check
that and error out if the unsafe function is invoked in parallel mode.
It has been observed that we were calling some unsafe functions in
parallel-mode in the regression tests which is caught by such a check.

I think here the main challenge is to do a similar check for built-in
functions and one of the ideas to do that was to extend FmgrBuiltins
to cache that information. I see why that idea is not good and maybe
we can see if there is some other place where we already fetch pg_proc
for built-in functions and can we have such a check at that place? If
that is not feasible then we can probably have such a check just for
non-built-in functions as that seems straightforward.

--
With Regards,
Amit Kapila.