Thread: Accidentally parallel unsafe functions
Hi, I am currently looking into adding the correct parallel options to all functions in the extensions and I noticed that some built-in functions seems to have been marked as unsafe by accident. The main culprit is system_views.sql which redefines these functions and removes the parallel safe flag. I think this counts as a 9.6 bug unlike my work on adding the flags to all extensions which is for 9.7. I have attached a patch which marks them and all conversion functions as parallel safe. I also added the flag to ts_debug() when I was already editing system_views.sql, feel free to ignore that one if you like. Affected functions: - json_populate_record() - json_populate_recordset() - jsonb_insert() - jsonb_set() - make_interval() - parse_ident() - Loads of conversion functions Andreas
Attachment
Andreas Karlsson wrote: > Hi, > > I am currently looking into adding the correct parallel options to all > functions in the extensions and I noticed that some built-in functions seems > to have been marked as unsafe by accident. The main culprit is > system_views.sql which redefines these functions and removes the parallel > safe flag. Surely CREATE OR REPLACE should keep whatever the flag was, rather than ovewrite it with a bogus value if not specified? In other words IMO the CREATE OR REPLACE code needs changing, not system_views.sql. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Andreas Karlsson wrote:
>> I am currently looking into adding the correct parallel options to all
>> functions in the extensions and I noticed that some built-in functions seems
>> to have been marked as unsafe by accident. The main culprit is
>> system_views.sql which redefines these functions and removes the parallel
>> safe flag.
> Surely CREATE OR REPLACE should keep whatever the flag was, rather than
> ovewrite it with a bogus value if not specified? In other words IMO the
> CREATE OR REPLACE code needs changing, not system_views.sql.
Absolutely not! The definition of CREATE OR REPLACE is that at the end,
the state of the object is predictable from only what the command says.
This is not open for renegotiation.
regards, tom lane
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Andreas Karlsson wrote:
>> I am currently looking into adding the correct parallel options to all
>> functions in the extensions and I noticed that some built-in functions seems
>> to have been marked as unsafe by accident. The main culprit is
>> system_views.sql which redefines these functions and removes the parallel
>> safe flag.
> Surely CREATE OR REPLACE should keep whatever the flag was, rather than
> ovewrite it with a bogus value if not specified? In other words IMO the
> CREATE OR REPLACE code needs changing, not system_views.sql.
Absolutely not! The definition of CREATE OR REPLACE is that at the end,
the state of the object is predictable from only what the command says.
This is not open for renegotiation.
To whit:
"""
When CREATE OR REPLACE FUNCTION is used to replace an existing function, the ownership and permissions of the function do not change. All other function properties are assigned the values specified or implied in the command. You must own the function to replace it (this includes being a member of the owning role)."""
I'd interpret "specified or implied in the command" to mean exactly what Tom said - and it applies to "all [other] function properties".
The ownership bit is a nice side-effect since you can use superuser to replace existing functions that are already owned by another user. Those are the only implied dynamic of function creation that exists within the CREATE FUNCTION command - everything else is contained within the CREATE FUNCTION DDL.
David J.
On 04/30/2016 01:19 AM, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> Surely CREATE OR REPLACE should keep whatever the flag was, rather than
>> ovewrite it with a bogus value if not specified? In other words IMO the
>> CREATE OR REPLACE code needs changing, not system_views.sql.
>
> Absolutely not! The definition of CREATE OR REPLACE is that at the end,
> the state of the object is predictable from only what the command says.
> This is not open for renegotiation.
An example to support Tom is that it already works like the for other
options.
postgres=# CREATE FUNCTION f() RETURNS int LANGUAGE sql AS $$ SELECT 1
$$ SECURITY DEFINER;
CREATE FUNCTION
postgres=# SELECT pg_get_functiondef('f'::regproc); pg_get_functiondef
--------------------------------------- CREATE OR REPLACE FUNCTION public.f()+ RETURNS integer +
LANGUAGEsql + SECURITY DEFINER + AS $function$ SELECT 1 $function$ +
(1 row)
postgres=# CREATE OR REPLACE FUNCTION f() RETURNS int LANGUAGE sql AS $$
SELECT 1 $$;
CREATE FUNCTION
postgres=# SELECT pg_get_functiondef('f'::regproc); pg_get_functiondef
--------------------------------------- CREATE OR REPLACE FUNCTION public.f()+ RETURNS integer +
LANGUAGEsql + AS $function$ SELECT 1 $function$ +
(1 row)
Andreas
On Fri, Apr 29, 2016 at 6:06 PM, Andreas Karlsson <andreas@proxel.se> wrote: > I am currently looking into adding the correct parallel options to all > functions in the extensions and I noticed that some built-in functions seems > to have been marked as unsafe by accident. The main culprit is > system_views.sql which redefines these functions and removes the parallel > safe flag. > > I think this counts as a 9.6 bug unlike my work on adding the flags to all > extensions which is for 9.7. > > I have attached a patch which marks them and all conversion functions as > parallel safe. I also added the flag to ts_debug() when I was already > editing system_views.sql, feel free to ignore that one if you like. > > Affected functions: > > - json_populate_record() > - json_populate_recordset() > - jsonb_insert() > - jsonb_set() > - make_interval() > - parse_ident() > - Loads of conversion functions Hmm. The new pg_start_backup() is not parallel-safe. It's parallel-restricted, because it relies on backend-private state. I'll go fix that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Apr 29, 2016 at 6:06 PM, Andreas Karlsson <andreas@proxel.se> wrote: > I am currently looking into adding the correct parallel options to all > functions in the extensions and I noticed that some built-in functions seems > to have been marked as unsafe by accident. The main culprit is > system_views.sql which redefines these functions and removes the parallel > safe flag. > > I think this counts as a 9.6 bug unlike my work on adding the flags to all > extensions which is for 9.7. > > I have attached a patch which marks them and all conversion functions as > parallel safe. I also added the flag to ts_debug() when I was already > editing system_views.sql, feel free to ignore that one if you like. > > Affected functions: > > - json_populate_record() > - json_populate_recordset() > - jsonb_insert() > - jsonb_set() > - make_interval() > - parse_ident() > - Loads of conversion functions Committed all of this except for the bit about pg_start_backup, for which I committed a separate fix. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 05/03/2016 08:45 PM, Robert Haas wrote: > Committed all of this except for the bit about pg_start_backup, for > which I committed a separate fix. Thanks, and really good that you spotted the pg_start_backup() issue. Andreas