Re: Unexpected result from ALTER FUNCTION— looks like a bug

["David G. Johnston" <david.g.johnston@xxxxxxxxx>]

On Tue, Apr 19, 2022 at 7:47 PM Julien Rouhaud <rjuju123@xxxxxxxxx> wrote:

On Tue, Apr 19, 2022 at 07:21:19PM -0700, David G. Johnston wrote:
> On Tue, Apr 19, 2022 at 7:07 PM Bryn Llewellyn <bryn@xxxxxxxxxxxx> wrote:
>

> > *alter function s1.f()security invokerset timezone = 'UTC'stable*
> > *parallel safe*
> > *;*
> >
> > It brings this new status:
> >
> >
> >
> > * name | type | security |                        proconfig
> >         | volatility
> > |  parallel  ------+------+----------+---------------------------------------------------------+------------+------------ f
> >    | func | invoker  | {TimeZone=UTC}
> >    | stable     | restricted*
> >
> > This is the bug.
> >
>
> It has room for improvement from a user experience perspective.
>
> While I haven't experimented with this for confirmation, what you are
> proposing here (set + parallel safe) is an impossible runtime combination
> (semantic rule) but perfectly valid to write syntactically.  Your function
> must either be restricted or unsafe per the rules for specifying parallel
> mode.
>

 

That's not the problem here though, as you can still end up with the wanted
result using 2 queries.  Also, the PARALLEL part is entirely ignored, so if you
wanted to mark the function as PARALLEL UNSAFE because you're also doing a SET
that would make it incompatible it would also be ignored, same if you use a
RESET clause.

AFAICT the problem is that SET / RESET part is messing with the HeapTuple, so
you can't use the procForm reference afterwards.  Simply processing
parallel_item before set_items fixes the problem, as in the attached.

Thank you for the explanation.

Might I suggest the following:

diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 91f02a7eb2..2790c64121 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -1416,12 +1416,20 @@ AlterFunction(ParseState *pstate, AlterFunctionStmt *stmt)
                        elog(ERROR, "option \"%s\" not recognized", defel->defname);
        }
 
+       /*
+        * For each action, modify procForm to type-safely set the new value.
+        * However, because the SET clause is repeatable we handle it
+        * a bit differently, modifying the underlying tuple directly.  So
+        * make sure to leave that conditional block for last.
+        */
        if (volatility_item)
                procForm->provolatile = interpret_func_volatility(volatility_item);
        if (strict_item)
                procForm->proisstrict = boolVal(strict_item->arg);
        if (security_def_item)
                procForm->prosecdef = boolVal(security_def_item->arg);
+       if (parallel_item)
+               procForm->proparallel = interpret_func_parallel(parallel_item);
        if (leakproof_item)
        {
                procForm->proleakproof = boolVal(leakproof_item->arg);
@@ -1506,8 +1514,7 @@ AlterFunction(ParseState *pstate, AlterFunctionStmt *stmt)
                tup = heap_modify_tuple(tup, RelationGetDescr(rel),
                                                                repl_val, repl_null, repl_repl);
        }
-       if (parallel_item)
-               procForm->proparallel = interpret_func_parallel(parallel_item);
+       /* The previous block must come last because it modifies tup directly instead of via procForm */
 
        /* Do the update */
        CatalogTupleUpdate(rel, &tup->t_self, tup);

I placed the parallel_item block at the end of the four blocks that all have single line bodies to keep the consistency of that form.

David J.