On Wed, May 5, 2021 at 7:39 PM Robert Haas <robertmh...@gmail.com> wrote: > > On Tue, May 4, 2021 at 11:47 PM Greg Nancarrow <gregn4...@gmail.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.