Horton

Thread

Commits

  1. Fix breakage in AlterFunction().

  1. Unexpected result from ALTER FUNCTION— looks like a bug

    Bryn Llewellyn <bryn@yugabyte.com> — 2022-04-20T02:07:30Z

    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

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

    David G. Johnston <david.g.johnston@gmail.com> — 2022-04-20T02:21:19Z

    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.

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-20T02:39:37Z

    "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

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

    Julien Rouhaud <rjuju123@gmail.com> — 2022-04-20T02:47:07Z

    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.

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

    Julien Rouhaud <rjuju123@gmail.com> — 2022-04-20T02:49:03Z

    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.

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-20T03:06:30Z

    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

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

    Julien Rouhaud <rjuju123@gmail.com> — 2022-04-20T03:14:31Z

    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.

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

    David G. Johnston <david.g.johnston@gmail.com> — 2022-04-20T03:15:18Z

    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);

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.

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-20T03:32:08Z

    "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

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

    Bryn Llewellyn <bryn@yugabyte.com> — 2022-04-20T17:45:22Z

    > 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.

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

    David G. Johnston <david.g.johnston@gmail.com> — 2022-04-20T17:54:59Z

    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.

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

    David G. Johnston <david.g.johnston@gmail.com> — 2022-04-20T17:58:24Z

    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.

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=9130f8cbb91954f7a40de70c014c01b552df31da

David J.