> From: Tom Lane <[hidden email]>
> > [ raised eyebrow... ] I find it very hard to understand why that would
> > be necessary, or even a good idea. Not least because there's no spare
> > room there; you'd have to incur a substantial enlargement of the
> > array to add another flag. But also, that would indeed lock down
> > the value of the parallel-safety flag, and that seems like a fairly
> > bad idea.
> You're right, FmgrBuiltins is already fully packed (24 bytes on 64-bit machines). Enlarging the frequently accessed fmgr_builtins array may wreak unexpectedly large adverse effect on performance.
> I wanted to check the parallel safety of functions, which various objects (data type, index, trigger, etc.) come down to, in FunctionCallInvoke() and other few places. But maybe we skip the check for built-in functions. That's a matter of where we draw a line between where we check and where we don't.
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?
I think we want to have some safety checks for functions as we have
for transaction id in AssignTransactionId(), command id in
CommandCounterIncrement(), for write operations in
heap_prepare_insert(), etc. Is that correct?
> Amit Kapila <[hidden email]> writes:
> > On Wed, Apr 21, 2021 at 8:12 AM [hidden email] > > <[hidden email]> wrote:
> >> From: Tom Lane <[hidden email]>
> >>> [ raised eyebrow... ] I find it very hard to understand why that would
> >>> be necessary, or even a good idea.
> > 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.
So, are you suggesting that we should fetch the pg_proc row for
built-in functions as well for this purpose? If not, then how to
identify parallel safety of built-in functions in fmgr_info()?
Another idea could be that we check parallel safety of built-in
functions based on some static information. As we know the func_ids of
non-parallel-safe built-in functions, we can have a function
fmgr_builtin_parallel_safe() which check if the func_id is not one
among the predefined func_ids of non-parallel-safe built-in functions,
it returns true, otherwise, false. Then, we can call this new function
in fmgr_info for built-in functions.
On Fri, Apr 23, 2021 at 6:45 PM Tom Lane <[hidden email]> wrote:
> Greg Nancarrow <[hidden email]> writes:
> > I'm curious. The FmgrBuiltin struct includes the "strict" flag, so
> > that would "lock down the value" of the strict flag, wouldn't it?
> It does, but that's much more directly a property of the function's
> C code than parallel-safety is.
Isn't parallel safety also the C code property? I mean unless someone
changes the built-in function code, changing that property would be
dangerous. The other thing is even if a user is allowed to change one
function's property, how will they know which other functions are
called by that function and whether they are parallel-safe or not. For
example, say if the user wants to change the parallel safe property of
a built-in function brin_summarize_new_values, unless she changes its
code and the functions called by it like brin_summarize_range, it
would be dangerous. So, isn't it better to disallow changing parallel
safety for built-in functions?
Also, if the strict property of built-in functions is fixed
internally, why we allow users to change it and is that of any help?