Accidentally parallel unsafe functions

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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:​

​http://www.postgresql.org/docs/current/static/sql-createfunction.html​

​"""
​
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 +
LANGUAGE sql +
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 +
LANGUAGE sql +
AS $function$ SELECT 1 $function$ +

(1 row)

Andreas

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers