Unexpected result from ALTER FUNCTION— looks like a bug
SUMMARY
This part of the syntax diagram for "alter function":
ALTER FUNCTION name [ ( [ [ argmode ] [ argname ] argtype [, ...] ] ) ] action [ … ]
says that the first "action" can be followed (without punctuation) by zero, one, or many other actions. A semantic rule says that no particular action can be specified more than once. My tests used these possible actions:
SECURITY { INVOKER | DEFINER }
SET configuration_parameter TO value
IMMUTABLE | STABLE | VOLATILE
PARALLEL { UNSAFE | RESTRICTED | SAFE }
The values of the properties set this way can be seen with a suitable query against "pg_catalog.pg_proc". (See the complete testcase below.) Suppose that the history of events shows this status for the function s1.f():
name | type | security | proconfig | volatility | parallel
------+------+----------+---------------------------------------------------------+------------+------------
f | func | invoker | | volatile | unsafe
This statement:
alter function s1.f()
security definer
immutable
parallel restricted;
brings this new status:
name | type | security | proconfig | volatility | parallel
------+------+----------+---------------------------------------------------------+------------+------------
f | func | definer | | immutable | restricted
confirming that the three specified changes have been made using just a single "alter function" statement.
However, when "SET configuration_parameter" is specified along with other changes, then the "parallel" specification (but only this) is ignored. The other three specifications are honored.
alter function s1.f()
security invoker
set 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.
Notice that with "alter procedure", the semantic difference between a procedure and a function means that you cannot specify "parallel" here, and so you can't demonstrate the bug here.
SELF-CONTAINED, RE-RUNNABLE TESTCASE tested using PG Version 14.1
--------------------------------------------------------------------------------
-- demo.sql
-----------
\o spool.txt
\c postgres postgres
set client_min_messages = warning;
drop database if exists db;
create database db owner postgres;
\c db postgres
set client_min_messages = warning;
drop schema if exists public cascade;
create schema s1 authorization postgres;
\i prepare-qry.sql
create function s1.f()
returns int
language plpgsql
as $body$
begin
return 0;
end;
$body$;
\t off
execute qry;
alter function s1.f()
security definer
immutable
parallel restricted;
\t on
execute qry;
-- Here is the bug. The test is meaningful only for a function.
alter function s1.f()
security invoker
set timezone = 'UTC'
stable
parallel safe;
execute qry;
\o
--------------------------------------------------------------------------------
-- prepare-qry.sql
------------------
drop view if exists s1.subprograms cascade;
create view s1.subprograms(
name,
pronamespace,
type,
security,
proconfig,
volatility,
parallel)
as
select
proname::text as name,
pronamespace::regnamespace::text,
case prokind
when 'a' then 'agg'
when 'w' then 'window'
when 'p' then 'proc'
else 'func'
end,
case
when prosecdef then 'definer'
else 'invoker'
end,
coalesce(proconfig::text, '') as proconfig,
case
when provolatile = 'i' then 'immutable'
when provolatile = 's' then 'stable'
when provolatile = 'v' then 'volatile'
end,
case
when proparallel = 'r' then 'restricted'
when proparallel = 's' then 'safe'
when proparallel = 'u' then 'unsafe'
end
from pg_catalog.pg_proc
where
proowner::regrole::text = 'postgres' and
pronamespace::regnamespace::text = 's1' and
pronargs = 0;
prepare qry as
select
rpad(name, 4) as name,
rpad(type, 4) as type,
rpad(security, 8) as security,
rpad(proconfig, 55) as proconfig,
rpad(volatility, 10) as volatility,
rpad(parallel, 10) as parallel
from s1.subprograms
where type in ('func', 'proc')
and pronamespace::regnamespace::text = 's1'
order by name;
--------------------------------------------------------------------------------
spool.txt
---------
name | type | security | proconfig | volatility | parallel
------+------+----------+---------------------------------------------------------+------------+------------
f | func | invoker | | volatile | unsafe
f | func | definer | | immutable | restricted
f | func | invoker | {TimeZone=UTC} | stable | restricted
On Tue, Apr 19, 2022 at 7:07 PM Bryn Llewellyn <bryn@yugabyte.com> wrote:
*SUMMARY*
This part of the syntax diagram for "alter function":
*ALTER FUNCTION name [ ( [ [ argmode ] [ argname ] argtype [, ...] ] ) ]
action [ … ]*says that the first "action" can be followed (without punctuation) by
zero, one, or many other actions. A semantic rule says that no particular
action can be specified more than once. My tests used these possible
actions:
*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.
If this is indeed what is happening then the documentation should make note
of it. Whether the server should emit a notice or warning in this
situation is less clear. I'm doubting we would introduce an error at this
point but probably should have when parallelism was first added to the
system.
David J.
"David G. Johnston" <david.g.johnston@gmail.com> writes:
On Tue, Apr 19, 2022 at 7:07 PM Bryn Llewellyn <bryn@yugabyte.com>
wrote:This is the bug.
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.
I'm not sure that that's actually disallowed. In any case, Bryn's
right, the combination of a SET clause and a PARALLEL clause is
implemented incorrectly in AlterFunction. Careless coding :-(
regards, tom lane
Hi,
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:
*SUMMARY*
This part of the syntax diagram for "alter function":
*ALTER FUNCTION name [ ( [ [ argmode ] [ argname ] argtype [, ...] ] ) ]
action [ … ]*says that the first "action" can be followed (without punctuation) by
zero, one, or many other actions. A semantic rule says that no particular
action can be specified more than once. My tests used these possible
actions:*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.If this is indeed what is happening then the documentation should make note
of it. Whether the server should emit a notice or warning in this
situation is less clear. I'm doubting we would introduce an error at this
point but probably should have when parallelism was first added to the
system.
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.
On Wed, Apr 20, 2022 at 10:47:07AM +0800, Julien Rouhaud wrote:
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.
This time with the file.
Attachments:
fix_alter_function.difftext/plain; charset=us-asciiDownload+2-2
Julien Rouhaud <rjuju123@gmail.com> writes:
On Wed, Apr 20, 2022 at 10:47:07AM +0800, Julien Rouhaud wrote:
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.
This time with the file.
Yeah, I arrived at the same fix. Another possibility would be to
make the procForm pointer valid again after heap_modify_tuple,
but that seemed like it'd add more code for no really good reason.
regards, tom lane
On Tue, Apr 19, 2022 at 11:06:30PM -0400, Tom Lane wrote:
Julien Rouhaud <rjuju123@gmail.com> writes:
On Wed, Apr 20, 2022 at 10:47:07AM +0800, Julien Rouhaud wrote:
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.This time with the file.
Yeah, I arrived at the same fix. Another possibility would be to
make the procForm pointer valid again after heap_modify_tuple,
but that seemed like it'd add more code for no really good reason.
Yeah I agree. The comment you added seems enough as a future-proof security.
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. Yourfunction
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.
"David G. Johnston" <david.g.johnston@gmail.com> writes:
Might I suggest the following: + /* + * 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.
+ */
Actually, the reason proconfig is handled differently is that it's
a variable-length field, so it can't be represented in the C struct
that we overlay onto the catalog tuple to access the fixed-width
fields cheaply. I'm not sure that insisting that that stanza be
last is especially useful advice for future hackers, because someday
there might be more than one variable-length field that this function
needs to update.
regards, tom lane
tgl@sss.pgh.pa.us wrote:
david.g.johnston@gmail.com wrote:
Might I suggest the following...
Actually, the reason proconfig is handled differently is that it's a variable-length field, so it can't be represented in the C struct that we overlay onto the catalog tuple...
Thanks to all who responded. Tom also wrote this, earlier:
In any case, Bryn's right, the combination of a SET clause and a PARALLEL clause is implemented incorrectly in AlterFunction.
I'm taking what I've read in the responses to mean that the testcase I showed is considered to be evidence of a bug (i.e. there are no semantic restrictions) and that fix(es) are under consideration.
I agree that, as long as you know about the bug, it's trivial to achieve your intended effect using two successive "alter function" statements (underlining the fact that there are indeed no semantic restrictions). I hardly have to say that the point is the risk that you silently don't get what you ask for—and might then need a lot of effort (like I had to spend) to work out why.
On Wed, Apr 20, 2022 at 10:45 AM Bryn Llewellyn <bryn@yugabyte.com> wrote:
tgl@sss.pgh.pa.us wrote:
In any case, Bryn's right, the combination of a SET clause and a
PARALLEL clause is implemented incorrectly in AlterFunction.
I'm taking what I've read in the responses to mean that the testcase I
showed is considered to be evidence of a bug (i.e. there are no semantic
restrictions) and that fix(es) are under consideration.
The test case was good. I made an uninformed assumption that proved to be
untrue.
The patch was written and applied yesterday, at Tom's "Yeah, I arrived at
the same fix." email.
https://github.com/postgres/postgres/commit/344a225cb9d42f20df063e4d0e0d4559c5de7910
(I haven't figured out what the official way to reference a commit is, I
use the GitHub clone for research so there ya go).
David J.
On Wed, Apr 20, 2022 at 10:54 AM David G. Johnston <
david.g.johnston@gmail.com> wrote:
https://github.com/postgres/postgres/commit/344a225cb9d42f20df063e4d0e0d4559c5de7910
(I haven't figured out what the official way to reference a commit is, I
use the GitHub clone for research so there ya go).
Nevermind...not a huge fan of gitweb yet but I do have the commit from the
message sent to -committers.
David J.