Re: [HACKERS] Accidentally parallel unsafe functions
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
Re: [HACKERS] Accidentally parallel unsafe functions
On Fri, Apr 29, 2016 at 6:06 PM, 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. > > 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
Re: [HACKERS] Accidentally parallel unsafe functions
On Fri, Apr 29, 2016 at 6:06 PM, 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. > > 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
Re: [HACKERS] Accidentally parallel unsafe functions
On 04/30/2016 01:19 AM, Tom Lane wrote:
Alvaro Herrera 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
Re: [HACKERS] Accidentally parallel unsafe functions
On Fri, Apr 29, 2016 at 4:19 PM, Tom Lane wrote: > Alvaro Herrera 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.
Re: [HACKERS] Accidentally parallel unsafe functions
Alvaro Herrera 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
Re: [HACKERS] 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 Herrerahttp://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
[HACKERS] 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
commit 9afcc5f1ed22be18d69dc0b70a0f057a023cc5ec
Author: Andreas Karlsson
Date: Fri Apr 29 23:29:42 2016 +0200
Mark functions as parallel safe
- Conversion fucntions
- Functions which are redfined in system_views.sql
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index d3cc848..e08bc67 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -890,7 +890,7 @@ FROM pg_catalog.ts_parse(
) AS tt
WHERE tt.tokid = parse.tokid
$$
-LANGUAGE SQL STRICT STABLE;
+LANGUAGE SQL STRICT STABLE PARALLEL SAFE;
COMMENT ON FUNCTION ts_debug(regconfig,text) IS
'debug function for text search configuration';
@@ -906,7 +906,7 @@ RETURNS SETOF record AS
$$
SELECT * FROM pg_catalog.ts_debug( pg_catalog.get_current_ts_config(), $1);
$$
-LANGUAGE SQL STRICT STABLE;
+LANGUAGE SQL STRICT STABLE PARALLEL SAFE;
COMMENT ON FUNCTION ts_debug(text) IS
'debug function for current text search configuration';
@@ -922,17 +922,17 @@ COMMENT ON FUNCTION ts_debug(text) IS
CREATE OR REPLACE FUNCTION
pg_start_backup(label text, fast boolean DEFAULT false, exclusive boolean DEFAULT true)
- RETURNS pg_lsn STRICT VOLATILE LANGUAGE internal AS 'pg_start_backup';
+ RETURNS pg_lsn STRICT VOLATILE LANGUAGE internal AS 'pg_start_backup' PARALLEL SAFE;
-- legacy definition for compatibility with 9.3
CREATE OR REPLACE FUNCTION
json_populate_record(base anyelement, from_json json, use_json_as_text boolean DEFAULT false)
- RETURNS anyelement LANGUAGE internal STABLE AS 'json_populate_record';
+ RETURNS anyelement LANGUAGE internal STABLE AS 'json_populate_record' PARALLEL SAFE;
-- legacy definition for compatibility with 9.3
CREATE OR REPLACE FUNCTION
json_populate_recordset(base anyelement, from_json json, use_json_as_text boolean DEFAULT false)
- RETURNS SETOF anyelement LANGUAGE internal STABLE ROWS 100 AS 'json_populate_recordset';
+ RETURNS SETOF anyelement LANGUAGE internal STABLE ROWS 100 AS 'json_populate_recordset' PARALLEL SAFE;
CREATE OR REPLACE FUNCTION pg_logical_slot_get_changes(
IN slot_name name, IN upto_lsn pg_lsn, IN upto_nchanges int, VARIADIC options text[] DEFAULT '{}',
@@ -980,7 +980,7 @@ CREATE OR REPLACE FUNCTION
secs double precision DEFAULT 0.0)
RETURNS interval
LANGUAGE INTERNAL
-STRICT IMMUTABLE
+STRICT IMMUTABLE PARALLEL SAFE
AS 'make_interval';
CREATE OR REPLACE FUNCTION
@@ -988,14 +988,14 @@ CREATE OR REPLACE FUNCTION
create_if_missing boolean DEFAULT true)
RETURNS jsonb
LANGUAGE INTERNAL
-STRICT IMMUTABLE
+STRICT IMMUTABLE PARALLEL SAFE
AS 'jsonb_set';
CREATE OR REPLACE FUNCTION
parse_ident(str text, strict boolean DEFAULT true)
RETURNS text[]
LANGUAGE INTERNAL
-STRICT IMMUTABLE
+STRICT IMMUTABLE PARALLEL SAFE
AS 'parse_ident';
CREATE OR REPLACE FUNCTION
@@ -1003,7 +1003,7 @@ CREATE OR REPLACE FUNCTION
insert_after boolean DEFAULT false)
RETURNS jsonb
LANGUAGE INTERNAL
-STRICT IMMUTABLE
+STRICT IMMUTABLE PARALLEL SAFE
AS 'jsonb_insert';
-- The default permissions for functions mean that anyone can execute them.
diff --git a/src/backend/utils/mb/conversion_procs/Makefile b/src/backend/utils/mb/conversion_procs/Makefile
index 8b97803..879467e 100644
--- a/src/backend/utils/mb/conversion_procs/Makefile
+++ b/src/backend/utils/mb/conversion_procs/Makefile
@@ -173,7 +173,7 @@ $(SQLSCRIPT): Makefile
func=$$1; shift; \
obj=$$1; shift; \
echo "-- $$se --> $$de"; \
- echo "CREATE OR REPLACE FUNCTION $$func (INTEGER, INTEGER, CSTRING, INTERNAL, INTEGER) RETURNS VOID AS '$$"libdir"/$$obj', '$$func' LANGUAGE C STRICT;"; \
+ echo "CREATE OR REPLACE FUNCTION $$func (INTEGER, INTEGER, CSTRING, INTERNAL, INTEGER) RETURNS VOID AS '$$"libdir"/$$obj', '$$func' LANGUAGE C STRICT PARALLEL SAFE;"; \
echo "COMMENT ON FUNCTION $$func(INTEGER, INTEGER, CSTRING, INTERNAL, INTEGER) IS 'internal conversion function for $$se to $$de';"; \
echo "DROP CONVERSION pg_catalog.$$name;"; \
echo "CREATE DEFAULT CONVERSION pg_catalog.$$name FOR '$$se' TO '$$de' FROM $$func;"; \
--
Sent via pgsql-hackers maili
