Parallel safety tagging of extension functions

On Thu, May 19, 2016 at 05:50:01PM -0400, Andreas Karlsson wrote:

Hi,

I have gone through all our extensions and tried to tag all functions
correctly according to their parallel safety.

I also did the same for the aggregate functions in a second patch, and for
min(citext)/max(citext) set a COMBINEFUNC.

The changes for the functions is attached as one huge patch. Feel free to
suggest a way to split it up or change it in any way if that would make it
easier to review/apply.

Some open questions:

- How should we modify the aggregate functions when upgrading extensions?
ALTER AGGREGATE cannot change COMBINEFUNC or PARALLEL.

At the moment, it basically changes namespace and ownership but
doesn't touch the actual implementation. Maybe it should be able to
tweak those. A brief check implies that it's not a gigantic amount of
work. Is it worth making ALTER AGGREGATE support the things CREATE
AGGREGATE does?

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

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

On Thu, May 19, 2016 at 5:50 PM, Andreas Karlsson <andreas@proxel.se> wrote:

I have gone through all our extensions and tried to tag all functions
correctly according to their parallel safety.

I also did the same for the aggregate functions in a second patch, and for
min(citext)/max(citext) set a COMBINEFUNC.

The changes for the functions is attached as one huge patch. Feel free to
suggest a way to split it up or change it in any way if that would make it
easier to review/apply.

Some open questions:

- How should we modify the aggregate functions when upgrading extensions?
ALTER AGGREGATE cannot change COMBINEFUNC or PARALLEL. My current patch
updates the system catalogs directly, which should be safe in this case, but
is this an acceptable solution?

- Do you think we should add PARALLEL UNSAFE to the functions which we know
are unsafe to make it obvious that it is intentional?

- I have not added the parallel tags to the functions used by our procedural
languages. Should we do so?

- I have marked uuid-ossp, chkpass_in() and pgcrypto functions which
generate random data as safe, despite that they depend on state in the
backend. My reasoning is that, especially for the pgcrypto functions, that
nobody should not rely on the PRNG state. For uuid-ossp I am on the fence.

- I have touched a lot of legacy libraries, like tsearch2 and the spi/*
stuff. Is that a good idea?

- I decided to ignore that isn_weak() exists (and would make all input
functions PARALLEL RESTRICTED) since it is only there is ISN_WEAK_MODE is
defined. Is that ok?

I guess my first question is whether we have consensus on the release
into which we should put this. Some people (Noah, among others)
thought it should wait because we're after feature freeze, while
others thought we should do it now. If we're going to try to get this
into 9.6, I'll work on reviewing this sooner rather than later, but if
we're not going to do that I'm going to postpone dealing with it until
after we branch.

--
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 5/20/16 7:37 PM, Robert Haas wrote:

I guess my first question is whether we have consensus on the release
into which we should put this. Some people (Noah, among others)
thought it should wait because we're after feature freeze, while
others thought we should do it now. If we're going to try to get this
into 9.6, I'll work on reviewing this sooner rather than later, but if
we're not going to do that I'm going to postpone dealing with it until
after we branch.

Sounds to me that this is part of the cleanup of a 9.6 feature and
should be in that release.

--
Peter Eisentraut 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

On Fri, May 20, 2016 at 10:30 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 5/20/16 7:37 PM, Robert Haas wrote:

I guess my first question is whether we have consensus on the release
into which we should put this. Some people (Noah, among others)
thought it should wait because we're after feature freeze, while
others thought we should do it now. If we're going to try to get this
into 9.6, I'll work on reviewing this sooner rather than later, but if
we're not going to do that I'm going to postpone dealing with it until
after we branch.

Sounds to me that this is part of the cleanup of a 9.6 feature and should be
in that release.

Yes, I agree. By the way, the patch completely ignores the fact that
some of the modules already had a version bump in the 9.6 development
cycle, like pageinpect. You don't need to create a new version script
in such cases.
--
Michael

--
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/20/2016 11:45 PM, Michael Paquier wrote:

Yes, I agree. By the way, the patch completely ignores the fact that
some of the modules already had a version bump in the 9.6 development
cycle, like pageinpect. You don't need to create a new version script
in such cases.

I assumed this was too large change after beta1 had been released, but
if people feel otherwise I have no problem adapting this patch for 9.6.

Andreas

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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 5/20/16 7:37 PM, Robert Haas wrote:

I guess my first question is whether we have consensus on the release
into which we should put this. Some people (Noah, among others)
thought it should wait because we're after feature freeze, while
others thought we should do it now. If we're going to try to get this
into 9.6, I'll work on reviewing this sooner rather than later, but if
we're not going to do that I'm going to postpone dealing with it until
after we branch.

Sounds to me that this is part of the cleanup of a 9.6 feature and
should be in that release.

Yes, let's fix it. This will also take care of the questions about
whether the GIN/GIST opclass tweaks I made a few months ago require
module version bumps.

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

Another question which I thought of is what we should do with functions
like pg_file_write() in adminpack.

While it is perfectly fine to modify files from the parallel workers, a
user could get race conditions if he tries to modify the same file
multiple times. Is this a kind of problem the PARALLEL tagging should
try to prevent, or is that something we should leave to the user?

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 Sat, May 21, 2016 at 1:01 PM, Andreas Karlsson <andreas@proxel.se> wrote:

Another question which I thought of is what we should do with functions like
pg_file_write() in adminpack.

While it is perfectly fine to modify files from the parallel workers, a user
could get race conditions if he tries to modify the same file multiple
times. Is this a kind of problem the PARALLEL tagging should try to prevent,
or is that something we should leave to the user?

Having multiple processes trying to write to a file on Windows is no
good either and would need some extra logic with EventWaitHandle().
I'd rather have those be marked as unsafe for now.
--
Michael

--
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/21/2016 11:45 AM, Tom Lane wrote:

Yes, let's fix it. This will also take care of the questions about
whether the GIN/GIST opclass tweaks I made a few months ago require
module version bumps.

Do you have any idea what the best way to add these tweaks to the
upgrade scripts would be?

My immediate thought is first doing an UPDATE of pg_proc and then
updating the catcache with CREATE OR REPLACE with the new arguments.
Does that work? Is there a less ugly way to accomplish this?

Example:

UPDATE pg_proc SET proargtypes = ('internal'::regtype::oid || ' ' ||
'internal'::regtype::oid)::oidvector
WHERE proname = 'gbt_oid_union'
AND proargtypes = ('bytea'::regtype::oid || ' ' ||
'internal'::regtype::oid)::oidvector
AND pronamespace = current_schema()::regnamespace;

CREATE OR REPLACE FUNCTION gbt_oid_union(internal, internal)
RETURNS gbtreekey8
AS 'MODULE_PATHNAME'
LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;

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 Sat, May 21, 2016 at 6:16 PM, Andreas Karlsson <andreas@proxel.se> wrote:

My immediate thought is first doing an UPDATE of pg_proc and then updating
the catcache with CREATE OR REPLACE with the new arguments. Does that work?
Is there a less ugly way to accomplish this?

Example:

UPDATE pg_proc SET proargtypes = ('internal'::regtype::oid || ' ' ||
'internal'::regtype::oid)::oidvector
WHERE proname = 'gbt_oid_union'
AND proargtypes = ('bytea'::regtype::oid || ' ' ||
'internal'::regtype::oid)::oidvector
AND pronamespace = current_schema()::regnamespace;

CREATE OR REPLACE FUNCTION gbt_oid_union(internal, internal)
RETURNS gbtreekey8
AS 'MODULE_PATHNAME'
LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;

Isn't it better to just drop and recreate the function? pageinspect
did so for example for heap_page_items in 1.4 to update its OUT
arguments.
--
Michael

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

Michael Paquier <michael.paquier@gmail.com> writes:

On Sat, May 21, 2016 at 6:16 PM, Andreas Karlsson <andreas@proxel.se> wrote:

My immediate thought is first doing an UPDATE of pg_proc and then updating
the catcache with CREATE OR REPLACE with the new arguments. Does that work?
Is there a less ugly way to accomplish this?

Isn't it better to just drop and recreate the function? pageinspect
did so for example for heap_page_items in 1.4 to update its OUT
arguments.

You'd have to alter the index opfamily to disconnect the function from it,
drop/recreate the function, then re-add it to the opfamily. Kind of icky,
but probably better than the alternatives.

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

Tom Lane wrote:

Michael Paquier <michael.paquier@gmail.com> writes:

On Sat, May 21, 2016 at 6:16 PM, Andreas Karlsson <andreas@proxel.se> wrote:

My immediate thought is first doing an UPDATE of pg_proc and then updating
the catcache with CREATE OR REPLACE with the new arguments. Does that work?
Is there a less ugly way to accomplish this?

Isn't it better to just drop and recreate the function? pageinspect
did so for example for heap_page_items in 1.4 to update its OUT
arguments.

You'd have to alter the index opfamily to disconnect the function from it,
drop/recreate the function, then re-add it to the opfamily. Kind of icky,
but probably better than the alternatives.

What happens if the upgraded database contains indexes using those
opfamilies? I suppose getting such indexes dropped because of ALTER
EXTENSION UPDATE is not very nice.

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

Tom Lane wrote:

You'd have to alter the index opfamily to disconnect the function from it,
drop/recreate the function, then re-add it to the opfamily. Kind of icky,
but probably better than the alternatives.

What happens if the upgraded database contains indexes using those
opfamilies? I suppose getting such indexes dropped because of ALTER
EXTENSION UPDATE is not very nice.

Sure, that's why we mustn't drop and recreate the whole opfamily.
But we can do ALTER OPERATOR FAMILY ... DROP ... without causing
dependent indexes to be dropped. Semi-bad things would happen if
someone tried to access such an index partway through; but as long
as the extension upgrade script itself doesn't do that, it should
be okay. We run extension scripts as single transactions so the
change should appear atomic.

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, May 20, 2016 at 11:45 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Sounds to me that this is part of the cleanup of a 9.6 feature and should be
in that release.

Yes, I agree. By the way, the patch completely ignores the fact that
some of the modules already had a version bump in the 9.6 development
cycle, like pageinpect. You don't need to create a new version script
in such cases.

I think now that beta1 has shipped we would want to bump the version either way.

--
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 Sat, May 21, 2016 at 1:01 PM, Andreas Karlsson <andreas@proxel.se> wrote:

Another question which I thought of is what we should do with functions like
pg_file_write() in adminpack.

While it is perfectly fine to modify files from the parallel workers, a user
could get race conditions if he tries to modify the same file multiple
times. Is this a kind of problem the PARALLEL tagging should try to prevent,
or is that something we should leave to the user?

I think there's little value in marking such things parallel-safe,
even though by some definition they may be. Parallelizing queries
involving pg_file_write() is not really a useful thing to do. What we
really want to do is nail the functions that people might be likely to
use as scan quals, plus any generally useful aggregates.

--
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 Thu, May 19, 2016 at 5:50 PM, Andreas Karlsson <andreas@proxel.se> wrote:

- How should we modify the aggregate functions when upgrading extensions?
ALTER AGGREGATE cannot change COMBINEFUNC or PARALLEL. My current patch
updates the system catalogs directly, which should be safe in this case, but
is this an acceptable solution?

I'd rather extend see us ALTER AGGREGATE to do this.

- Do you think we should add PARALLEL UNSAFE to the functions which we know
are unsafe to make it obvious that it is intentional?

That seems likely unnecessary churn from here.

- I have not added the parallel tags to the functions used by our procedural
languages. Should we do so?

I don't think that accomplishes anything.

- I have marked uuid-ossp, chkpass_in() and pgcrypto functions which
generate random data as safe, despite that they depend on state in the
backend. My reasoning is that, especially for the pgcrypto functions, that
nobody should not rely on the PRNG state. For uuid-ossp I am on the fence.

random() is marked parallel-restricted because of setseed(). If
there's no equivalent for other random number generators then I think
they can be construed as safe.

- I have touched a lot of legacy libraries, like tsearch2 and the spi/*
stuff. Is that a good idea?

I don't know. It doesn't seem particularly important, but I can't
immediately see a reason not to do it either. You could argue it
might allow for better test coverage...

- I decided to ignore that isn_weak() exists (and would make all input
functions PARALLEL RESTRICTED) since it is only there is ISN_WEAK_MODE is
defined. Is that ok?

That seems fine.

--
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 Tue, May 24, 2016 at 8:41 PM, Robert Haas <robertmhaas@gmail.com> wrote:

- Do you think we should add PARALLEL UNSAFE to the functions which we know
are unsafe to make it obvious that it is intentional?

That seems likely unnecessary churn from here.

A general point here is that there's no point in marking a function
PARALLEL SAFE unless it's going to be referenced in a query. So for
example I'm pretty sure the parallel markings on blhandler() don't
matter at all, and therefore there's no need to update the bloom
contrib module. Yeah, that function might get called, but it's not
going to be mentioned textually in the query.

I think this patch can get somewhat smaller if you update it that way.
I suggest merging the function and aggregate stuff together and
instead splitting this by contrib module.

--
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/25/2016 02:34 AM, Robert Haas wrote:

On Fri, May 20, 2016 at 11:45 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Sounds to me that this is part of the cleanup of a 9.6 feature and should be
in that release.

Yes, I agree. By the way, the patch completely ignores the fact that
some of the modules already had a version bump in the 9.6 development
cycle, like pageinpect. You don't need to create a new version script
in such cases.

I think now that beta1 has shipped we would want to bump the version either way.

I am fine with doing it either way. I will leave it as new versions for
everything now then and if people want to I can merge the two upgrade
scripts.

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 05/25/2016 02:41 AM, Robert Haas wrote:

On Thu, May 19, 2016 at 5:50 PM, Andreas Karlsson <andreas@proxel.se> wrote:

- How should we modify the aggregate functions when upgrading extensions?
ALTER AGGREGATE cannot change COMBINEFUNC or PARALLEL. My current patch
updates the system catalogs directly, which should be safe in this case, but
is this an acceptable solution?

I'd rather extend see us ALTER AGGREGATE to do this.

Wouldn't that prevent this from going into 9.6? I do not think changing
ALTER AGGREGATE is 9.6 materials.

Andreas

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