Re: Unexpected result from ALTER FUNCTION— looks like a bug
On Wed, Apr 20, 2022 at 10:54 AM David G. Johnston < david.g.johns...@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.
Re: Unexpected result from ALTER FUNCTION— looks like a bug
On Wed, Apr 20, 2022 at 10:45 AM Bryn Llewellyn wrote: > > t...@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.
Re: Unexpected result from ALTER FUNCTION— looks like a bug
> t...@sss.pgh.pa.us wrote: > >> david.g.johns...@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.
Re: Re: Unexpected result from ALTER FUNCTION— looks like a bug
"David G. Johnston" 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
Re: Unexpected result from ALTER FUNCTION— looks like a bug
On Tue, Apr 19, 2022 at 7:47 PM Julien Rouhaud 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
> 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.
Re: Unexpected result from ALTER FUNCTION— looks like a bug
On Tue, Apr 19, 2022 at 11:06:30PM -0400, Tom Lane wrote: > Julien Rouhaud 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.
Re: Unexpected result from ALTER FUNCTION— looks like a bug
Julien Rouhaud 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
Re: Unexpected result from ALTER FUNCTION— looks like a bug
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.
diff --git a/src/backend/commands/functioncmds.c
b/src/backend/commands/functioncmds.c
index 91f02a7eb2..c227fbde19 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -1472,6 +1472,8 @@ AlterFunction(ParseState *pstate, AlterFunctionStmt *stmt)
procForm->prosupport = newsupport;
}
+ if (parallel_item)
+ procForm->proparallel = interpret_func_parallel(parallel_item);
if (set_items)
{
Datum datum;
@@ -1506,8 +1508,6 @@ 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);
/* Do the update */
CatalogTupleUpdate(rel, &tup->t_self, tup);
Re: Unexpected result from ALTER FUNCTION— looks like a bug
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 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.
Re: Re: Unexpected result from ALTER FUNCTION— looks like a bug
"David G. Johnston" writes: > On Tue, Apr 19, 2022 at 7:07 PM Bryn Llewellyn > 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
Re: Unexpected result from ALTER FUNCTION— looks like a bug
On Tue, Apr 19, 2022 at 7:07 PM Bryn Llewellyn 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.
