Re: Unexpected result from ALTER FUNCTION— looks like a bug - Mailing list pgsql-general
| From | David G. Johnston |
|---|---|
| Subject | Re: Unexpected result from ALTER FUNCTION— looks like a bug |
| Date | |
| Msg-id | CAKFQuwbqrC7VygfVuMhw_boY2Xf1jMUfG_PUo=3H4N0VZpR6gA@mail.gmail.com Whole thread Raw |
| In response to | Re: Unexpected result from ALTER FUNCTION— looks like a bug (Julien Rouhaud <rjuju123@gmail.com>) |
| Responses |
Re: Re: Unexpected result from ALTER FUNCTION— looks like a bug
(Tom Lane <tgl@sss.pgh.pa.us>)
|
| List | pgsql-general |
On Tue, Apr 19, 2022 at 7:47 PM Julien Rouhaud <rjuju123@gmail.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@yugabyte.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);
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.
pgsql-general by date: