Thread: Accidentally parallel unsafe functions

Accidentally parallel unsafe functions

From
Andreas Karlsson
Date:
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

Re: Accidentally parallel unsafe functions

From
Alvaro Herrera
Date:
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



Re: Accidentally parallel unsafe functions

From
Tom Lane
Date:
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



Re: Accidentally parallel unsafe functions

From
"David G. Johnston"
Date:
On Fri, Apr 29, 2016 at 4:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
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.

Re: Accidentally parallel unsafe functions

From
Andreas Karlsson
Date:
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



Re: Accidentally parallel unsafe functions

From
Robert Haas
Date:
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



Re: Accidentally parallel unsafe functions

From
Robert Haas
Date:
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



Re: Accidentally parallel unsafe functions

From
Andreas Karlsson
Date:
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