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

On Tue, Apr 19, 2022 at 7:47 PM Julien Rouhaud <rjuju123(at)gmail(dot)com> 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(at)yugabyte(dot)com>
> 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.