Обсуждение: Parallel safety tagging of extension functions

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. 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?

Andreas

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

On 05/25/2016 03:09 AM, Robert Haas wrote:
> 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.

Ok, then I can avoid touching all functions which are only called by 
operator classes, tsearch, pls, fdws, etc. Which also means that there 
is no need to care about Tom's changes to the signatures of GIN and GiST 
support functions.

I am also fine with splitting it per extension.

Thanks for the feedback. I aim to find the time to incorporate it in a 
new set of patches the upcoming couple of days.

Andreas

Andreas Karlsson <andreas@proxel.se> writes:
> On 05/25/2016 02:41 AM, Robert Haas wrote:
>> 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.

Well, it's debatable --- but if the patch to do it is small and the
alternatives are really ugly, that would be an acceptable choice IMO.
Certainly we'd want to add that capability eventually anyway.
        regards, tom lane

Andreas Karlsson <andreas@proxel.se> writes:
> Ok, then I can avoid touching all functions which are only called by 
> operator classes, tsearch, pls, fdws, etc. Which also means that there 
> is no need to care about Tom's changes to the signatures of GIN and GiST 
> support functions.

I think as long as you already did the work, we should keep those updates.
I'm not totally convinced by Alexander's argument that those changes pose
a future hazard, but I'm not convinced he's wrong either.  If we're going
to be bumping a lot of contrib module versions anyway, it'd be silly to
take the risk that that's not a problem.
        regards, tom lane

All,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Andreas Karlsson <andreas@proxel.se> writes:
> > On 05/25/2016 02:41 AM, Robert Haas wrote:
> >> 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.
>
> Well, it's debatable --- but if the patch to do it is small and the
> alternatives are really ugly, that would be an acceptable choice IMO.
> Certainly we'd want to add that capability eventually anyway.

I tend to agree with Tom on this.  This should really have been included
in the earlier patches, but there's no help for that and if it's a small
patch and the other options are far worse then we need to accept that
solution and move on.

Thanks!

Stephen

On 05/25/2016 03:37 AM, Tom Lane wrote:
> Andreas Karlsson <andreas@proxel.se> writes:
>> Ok, then I can avoid touching all functions which are only called by
>> operator classes, tsearch, pls, fdws, etc. Which also means that there
>> is no need to care about Tom's changes to the signatures of GIN and GiST
>> support functions.
>
> I think as long as you already did the work, we should keep those updates.
> I'm not totally convinced by Alexander's argument that those changes pose
> a future hazard, but I'm not convinced he's wrong either.  If we're going
> to be bumping a lot of contrib module versions anyway, it'd be silly to
> take the risk that that's not a problem.

So how to best change the function signatures? I do not think it is 
possible without locking indexes by just using the SQL commands. You 
cannot drop a function from the operator family without dropping the 
operator class first.

Is the correct solution to manually update pg_amop with a new value for 
amopmethod?

Andreas

Andreas Karlsson <andreas@proxel.se> writes:
> So how to best change the function signatures? I do not think it is 
> possible without locking indexes by just using the SQL commands. You 
> cannot drop a function from the operator family without dropping the 
> operator class first.

Ugh.  I had been thinking that you could use ALTER OPERATOR FAMILY DROP
FUNCTION, but these functions are not "loose" in the opfamily --- they're
assumed to be bound to the specific opclass.  So there's an opclass
dependency preventing them from getting dropped; and we have no SQL
commands for changing the contents of an opclass.  After some fooling
around, I couldn't find a way to do it without some manual catalog update
or other.

Given that, your original approach of manually updating proargtypes in the
existing pg_proc row for the functions may be the best way.  Anything else
is going to be more complicated and ultimately will still require at least
one direct catalog update.

Sometime we might want to think about making this sort of thing cleaner
with more ALTER commands, but post-beta is probably not the time for that;
it wouldn't be a small patch.
        regards, tom lane

On 05/31/2016 06:47 PM, Tom Lane wrote:
> Given that, your original approach of manually updating proargtypes in the
> existing pg_proc row for the functions may be the best way.  Anything else
> is going to be more complicated and ultimately will still require at least
> one direct catalog update.

It is the least ugly of all the ugly solutions I could think of. I have
attached a patch which fixes the signatures using this method. I use
CREATE OR REPLACE FUNCTION to update to catcache. What do you think? Is
it too ugly?

Andreas

Andreas Karlsson <andreas@proxel.se> writes:
> It is the least ugly of all the ugly solutions I could think of. I have 
> attached a patch which fixes the signatures using this method. I use 
> CREATE OR REPLACE FUNCTION to update to catcache. What do you think? Is 
> it too ugly?

I don't understand why you think you need the CREATE OR REPLACE FUNCTION
commands?  We only need to change proargtypes, and the updates did that.
The catcache can take care of itself.

I think it would be good practice to be more careful about
schema-qualifying all the pg_catalog table and type names.

I also think it's a bad idea to use to_regprocedure() rather than
a cast to regprocedure.  If the name isn't found, we want an error,
not a silent NULL result leading to no update occurring.
        regards, tom lane

On 06/01/2016 04:44 PM, Tom Lane wrote:
> Andreas Karlsson <andreas@proxel.se> writes:
>> It is the least ugly of all the ugly solutions I could think of. I have
>> attached a patch which fixes the signatures using this method. I use
>> CREATE OR REPLACE FUNCTION to update to catcache. What do you think? Is
>> it too ugly?
>
> I don't understand why you think you need the CREATE OR REPLACE FUNCTION
> commands?  We only need to change proargtypes, and the updates did that.
> The catcache can take care of itself.

Maybe I did something wrong (I try to avoid doing manual catalog 
updates), but when I tested it, I needed to run the CREATE OR REPLACE 
FUNCTION command to later be able to set the parallel safety. See the 
example below.

CREATE FUNCTION f(text) RETURNS int LANGUAGE SQL AS $$ SELECT 1 $$;

BEGIN;

UPDATE pg_proc SET proargtypes = 
array_to_string('{int,int}'::regtype[]::oid[], ' ')::oidvector
WHERE oid = to_regprocedure('f(text)')
AND pronamespace = current_schema()::regnamespace;

ALTER FUNCTION f(int, int) STRICT;

COMMIT;

vs

CREATE FUNCTION f(text) RETURNS int LANGUAGE SQL AS $$ SELECT 1 $$;

BEGIN;

UPDATE pg_proc SET proargtypes = 
array_to_string('{int,int}'::regtype[]::oid[], ' ')::oidvector
WHERE oid = to_regprocedure('f(text)')
AND pronamespace = current_schema()::regnamespace;

CREATE OR REPLACE FUNCTION f(int, int) RETURNS int LANGUAGE SQL AS $$ 
SELECT 1 $$;

ALTER FUNCTION f(int, int) STRICT;

COMMIT;

> I think it would be good practice to be more careful about
> schema-qualifying all the pg_catalog table and type names.

I do not think we generally schema qualify things in extension scripts 
and instead rely of the CREATE/ALTER EXTENSION commands to set the 
search path correct. Am I mistaken?

> I also think it's a bad idea to use to_regprocedure() rather than
> a cast to regprocedure.  If the name isn't found, we want an error,
> not a silent NULL result leading to no update occurring.

The reason I use to_regprocedure is so that these scripts work for 
people who have installed the extensions in beta1 and therefore only 
have the new signatures. If they run these scripts they would get an 
error if I used the cast. Is it ok if these scripts break for beta1 users?

Andreas

Andreas Karlsson <andreas@proxel.se> writes:
> On 06/01/2016 04:44 PM, Tom Lane wrote:
>> I don't understand why you think you need the CREATE OR REPLACE FUNCTION
>> commands?  We only need to change proargtypes, and the updates did that.
>> The catcache can take care of itself.

> Maybe I did something wrong (I try to avoid doing manual catalog 
> updates), but when I tested it, I needed to run the CREATE OR REPLACE 
> FUNCTION command to later be able to set the parallel safety. See the 
> example below.

In this particular example, the problem seems to be that you forgot to
update pronargs; it works for me when I add "pronargs = 2" to the UPDATE.
Your "working" example is actually creating a new function, not replacing
the old pg_proc entry.

BTW, it strikes me that the pronamespace tests in these queries are
redundant: the OID is unique, so what matters is the search path
in the regprocedure lookups.

> The reason I use to_regprocedure is so that these scripts work for 
> people who have installed the extensions in beta1 and therefore only 
> have the new signatures. If they run these scripts they would get an 
> error if I used the cast. Is it ok if these scripts break for beta1 users?

Ugh.  This is more of a mess than I thought.  I don't like update scripts
that might silently fail to do what they're supposed to, but leaving
beta1 users in the lurch is not very nice either.

I wonder ... could we get away with using regproc rather than
regprocedure?  These function names are probably unique anyway ...
        regards, tom lane

On 06/01/2016 05:15 PM, Tom Lane wrote:
> Andreas Karlsson <andreas@proxel.se> writes:
>> On 06/01/2016 04:44 PM, Tom Lane wrote:
>>> I don't understand why you think you need the CREATE OR REPLACE FUNCTION
>>> commands?  We only need to change proargtypes, and the updates did that.
>>> The catcache can take care of itself.
>
>> Maybe I did something wrong (I try to avoid doing manual catalog
>> updates), but when I tested it, I needed to run the CREATE OR REPLACE
>> FUNCTION command to later be able to set the parallel safety. See the
>> example below.
>
> In this particular example, the problem seems to be that you forgot to
> update pronargs; it works for me when I add "pronargs = 2" to the UPDATE.
> Your "working" example is actually creating a new function, not replacing
> the old pg_proc entry.
>
> BTW, it strikes me that the pronamespace tests in these queries are
> redundant: the OID is unique, so what matters is the search path
> in the regprocedure lookups.

Thanks, I have fixed this.

>> The reason I use to_regprocedure is so that these scripts work for
>> people who have installed the extensions in beta1 and therefore only
>> have the new signatures. If they run these scripts they would get an
>> error if I used the cast. Is it ok if these scripts break for beta1 users?
>
> Ugh.  This is more of a mess than I thought.  I don't like update scripts
> that might silently fail to do what they're supposed to, but leaving
> beta1 users in the lurch is not very nice either.
>
> I wonder ... could we get away with using regproc rather than
> regprocedure?  These function names are probably unique anyway ...

Yeah, I would have strongly preferred to be able to use the cast. All
these functions have unique names within the core, but there is the
small risk of someone adding a user function with the same name. I do
not like either option.

The attached patch still uses to_regprocedure, but I can change to using
::regproc if that is what you prefer.

Andreas

On 05/25/2016 03:32 AM, Tom Lane wrote:
> Andreas Karlsson <andreas@proxel.se> writes:
>> On 05/25/2016 02:41 AM, Robert Haas wrote:
>>> 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.
>
> Well, it's debatable --- but if the patch to do it is small and the
> alternatives are really ugly, that would be an acceptable choice IMO.
> Certainly we'd want to add that capability eventually anyway.

Looked at this quickly and I do not think adding it would be what I 
consider a small patch since we would essentially need to copy the 
validation logic from DefineAggregate and AggregateCreate and modify it 
to fit the alter case. I am leaning towards either either leaving the 
aggregate functions alone or updating the catalog manually.

Andreas

On Thu, Jun 2, 2016 at 7:36 AM, Andreas Karlsson <andreas@proxel.se> wrote:
> On 05/25/2016 03:32 AM, Tom Lane wrote:
>>
>> Andreas Karlsson <andreas@proxel.se> writes:
>>>
>>> On 05/25/2016 02:41 AM, Robert Haas wrote:
>>>>
>>>> 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.
>>
>>
>> Well, it's debatable --- but if the patch to do it is small and the
>> alternatives are really ugly, that would be an acceptable choice IMO.
>> Certainly we'd want to add that capability eventually anyway.
>
>
> Looked at this quickly and I do not think adding it would be what I consider
> a small patch since we would essentially need to copy the validation logic
> from DefineAggregate and AggregateCreate and modify it to fit the alter
> case. I am leaning towards either either leaving the aggregate functions
> alone or updating the catalog manually.

As this is proving to be a hassle, what would it cost to leave those
operator classes as-is for 9.6 and come up with a cleaner solution at
DDL level with 10.0? Then we could still focus on the other extensions
that have content that can be easily switched. That's more than 90% of
the things that need to marked with parallel-safe.
-- 
Michael

On 06/02/2016 01:41 AM, Michael Paquier wrote:> On Thu, Jun 2, 2016 at 7:36 AM, Andreas Karlsson <andreas@proxel.se> 
wrote:>> Looked at this quickly and I do not think adding it would be what I 
consider>> a small patch since we would essentially need to copy the validation 
logic>> from DefineAggregate and AggregateCreate and modify it to fit the alter>> case. I am leaning towards either
eitherleaving the aggregate functions>> alone or updating the catalog manually.>> As this is proving to be a hassle,
whatwould it cost to leave those> operator classes as-is for 9.6 and come up with a cleaner solution at> DDL level with
10.0?Then we could still focus on the other extensions> that have content that can be easily switched. That's more than
90%of> the things that need to marked with parallel-safe.
 

I think at least three of the four aggregate functions are little used, 
so I do not think many users would be affected. And only min(citext) and 
max(citext) can make use of the parallel aggregation.

The functions are:

min(citext)
max(citext)
int_array_aggregate(int4)
rewrite(tsquery[])

It would be nice if we had support for this in ALTER AGGREGATE in 9.6 
already since I bet that there are external extensions which want to 
take advantage of the parallel aggregation, but at least if I add this 
command I do not feel like it would be a minor patch. If people disagree 
and are fine with copying the validation logic, then I am prepare to do 
the work. I would just rather not rush this if there is no chance anyway 
that it will get into 9.6.

Andreas

Hi,

Here is the patch split into many small patches as you suggested. The
current patches are based on top of the patch which fixes the signatures
for gin and gist functions.

These patches only touch functions which never should be called
directly, so they are fine to skip. I decided to attach them anyway in
case you fell like applying them.

parallel-contrib-v2-bloom.patch.gz
parallel-contrib-v2-btree_gin.patch.gz
parallel-contrib-v2-btree_gist.patch.gz
parallel-contrib-v2-dict_int.patch.gz
parallel-contrib-v2-dict_xsyn.patch.gz
parallel-contrib-v2-file_fdw.patch.gz
parallel-contrib-v2-hstore_plperl.patch.gz
parallel-contrib-v2-hstore_plpython.patch.gz
parallel-contrib-v2-ltree_plpython.patch.gz
parallel-contrib-v2-postgres_fdw.patch.gz
parallel-contrib-v2-tsm_system_rows.patch.gz
parallel-contrib-v2-tsm_system_time.patch.gz

These two mostly concern triggers so i guess they could be skipped too.

parallel-contrib-v2-tcn.patch.gz
parallel-contrib-v2-spi.patch.gz

Andreas

On Fri, Jun 3, 2016 at 8:37 PM, Andreas Karlsson <andreas@proxel.se> wrote:
> Here is the patch split into many small patches as you suggested. The
> current patches are based on top of the patch which fixes the signatures for
> gin and gist functions.

Generally, I think that there's no point in changing many of these
contrib modules in this way.  There's no use that I can see, for
example, in running a parallel query using any of the adminpack
functions, so why mark them anything other than the default of
parallel-unsafe?

adminpack: Doesn't seem useful.
bloom: As you say, functions are never called directly, so there's no point.
btree_gin: As you say, functions are never called directly, so there's no point.
btree_gist: As you say, functions are never called directly, so
there's no point.
chkpass: Doesn't seem useful.
citext: Committed.
cube: I think we need a new extension version.
dblink: Isn't changing dblink_fdw_validator pointless?  The others I get.
earthdistance: Committed.
file_fdw: As you say, functions are never called directly, so there's no point.
fuzzystrmatch: Committed.
hstore: Does not apply for me.
hstore_plperl: As you say, functions are never called directly, so
there's no point.
hstore_plpython: As you say, functions are never called directly, so
there's no point.
intagg: Committed.
intarray: Does not apply for me.
lo: Committed.

More later.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 06/07/2016 05:44 PM, Robert Haas wrote:
> cube: I think we need a new extension version.
> hstore: Does not apply for me.
> intarray: Does not apply for me.

Those three and ltree, pg_trgm, and seg depend on my patch with fixes 
for the GiST/GIN function signatures in 
https://www.postgresql.org/message-id/574F091A.3050800@proxel.se. The 
reason for the dependency is that I also mark the those functions with 
changed signatures as parallel safe.

If that patch is not going to be applied I can easily fix those 6 
patches to not depend on the function signature patch.

Sorry for not making this dependency clear.

Andreas

On 06/07/2016 05:44 PM, Robert Haas wrote:
> adminpack: Doesn't seem useful.

The case I imagined was if someone would use these functions on the 
result from a slow CTE and would want the CTE to be executed in 
parallel. I have no idea if that is a realistic case, but I rarely use 
adminpack in my own work.

> chkpass: Doesn't seem useful.

Agreed.

> dblink: Isn't changing dblink_fdw_validator pointless?  The others I get.

Yeah, but since it is just one function I think it makes sense to change 
it when we already are bumping the version of the extension. I think it 
makes sense to skip whole extensions, like chkpass or bloom, but if it 
is just a few functions where it does not matter, why not tag them as 
safe? Personally I think the churn which really matters is if we have to 
bump the extension version or not.

Andreas

On Wed, Jun 8, 2016 at 9:24 AM, Andreas Karlsson <andreas@proxel.se> wrote:
>> dblink: Isn't changing dblink_fdw_validator pointless?  The others I get.
>
> Yeah, but since it is just one function I think it makes sense to change it
> when we already are bumping the version of the extension. I think it makes
> sense to skip whole extensions, like chkpass or bloom, but if it is just a
> few functions where it does not matter, why not tag them as safe? Personally
> I think the churn which really matters is if we have to bump the extension
> version or not.

I broadly agree with that, but I'm slightly wary about giving people
the idea that parallel-safety will be checked in cases where it really
will not.  The stuff that gets tested for parallel-safety is the stuff
actually mentioned in the query.  Indirectly-referenced stuff will not
get tested, but if we start marking it that way, then we might create
confusion.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, May 21, 2016 at 11:45 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.

Tom, there's a patch for this at
https://www.postgresql.org/message-id/574F091A.3050800@proxel.se which
I think you should review, since you were the one who made the tweaks
involved.  Any chance you can do that RSN?  If we want to get this
done at all, we should really get it into beta2.  Also, the patches
Andreas posted for applying parallel-safety markings apply on top of
that, so that needs to go in first, at least for the affected modules.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, May 21, 2016 at 11:45 AM, Tom Lane <tgl@sss.pgh.pa.us> 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.

> Tom, there's a patch for this at
> https://www.postgresql.org/message-id/574F091A.3050800@proxel.se which
> I think you should review, since you were the one who made the tweaks
> involved.  Any chance you can do that RSN?  If we want to get this
> done at all, we should really get it into beta2.  Also, the patches
> Andreas posted for applying parallel-safety markings apply on top of
> that, so that needs to go in first, at least for the affected modules.

OK, I'll take it.  You're done with Andreas' patches otherwise?
        regards, tom lane

On Thu, Jun 9, 2016 at 1:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Sat, May 21, 2016 at 11:45 AM, Tom Lane <tgl@sss.pgh.pa.us> 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.
>
>> Tom, there's a patch for this at
>> https://www.postgresql.org/message-id/574F091A.3050800@proxel.se which
>> I think you should review, since you were the one who made the tweaks
>> involved.  Any chance you can do that RSN?  If we want to get this
>> done at all, we should really get it into beta2.  Also, the patches
>> Andreas posted for applying parallel-safety markings apply on top of
>> that, so that needs to go in first, at least for the affected modules.
>
> OK, I'll take it.  You're done with Andreas' patches otherwise?

No.  I can go through the unaffected ones in the meantime, though, and
then come back to the affected ones after you deal with this.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, May 21, 2016 at 11:45 AM, Tom Lane <tgl@sss.pgh.pa.us> 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.

> Tom, there's a patch for this at
> https://www.postgresql.org/message-id/574F091A.3050800@proxel.se which
> I think you should review, since you were the one who made the tweaks
> involved.  Any chance you can do that RSN?

I've pushed this with some revisions to make the queries more
search-path-safe.  I'm not too happy with the safety of the queries
I see already present from the previous patches.  I think stuff
like this:

UPDATE pg_proc SET proparallel = 's'
WHERE oid = 'min(citext)'::regprocedure;

needs to be more like

UPDATE pg_catalog.pg_proc SET proparallel = 's'
WHERE oid = 'min(citext)'::pg_catalog.regprocedure;
        regards, tom lane

On Thu, Jun 9, 2016 at 4:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Sat, May 21, 2016 at 11:45 AM, Tom Lane <tgl@sss.pgh.pa.us> 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.
>
>> Tom, there's a patch for this at
>> https://www.postgresql.org/message-id/574F091A.3050800@proxel.se which
>> I think you should review, since you were the one who made the tweaks
>> involved.  Any chance you can do that RSN?
>
> I've pushed this with some revisions to make the queries more
> search-path-safe.  I'm not too happy with the safety of the queries
> I see already present from the previous patches.  I think stuff
> like this:
>
> UPDATE pg_proc SET proparallel = 's'
> WHERE oid = 'min(citext)'::regprocedure;
>
> needs to be more like
>
> UPDATE pg_catalog.pg_proc SET proparallel = 's'
> WHERE oid = 'min(citext)'::pg_catalog.regprocedure;

We could do that, but there's no guarantee that "min" or "citext"
resolve correctly either, is there?  Basically, the search-path-safety
of many of the scripts already in contrib looks pretty horrendous to
me.  For example:

CREATE VIEW pg_buffercache AS       SELECT P.* FROM pg_buffercache_pages() AS P       (bufferid integer, relfilenode
oid,reltablespace oid, reldatabase oid,        relforknumber int2, relblocknumber int8, isdirty bool, usagecount int2,
     pinning_backends int4);
 

Well, what guarantee have we that we'll get the right
pg_buffercache_pages() function?

CREATE FUNCTION earth() RETURNS float8
LANGUAGE SQL IMMUTABLE PARALLEL SAFE
AS 'SELECT ''6378168''::float8';

What guarantees we'll get the correct float8 type?

CREATE FUNCTION sec_to_gc(float8)
RETURNS float8
LANGUAGE SQL
IMMUTABLE STRICT
PARALLEL SAFE
AS 'SELECT CASE WHEN $1 < 0 THEN 0::float8 WHEN $1/(2*earth()) > 1
THEN pi()*earth() ELSE 2*earth()*asin($1/(2*earth())) END';

Don't even get me started.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-06-09 17:40:24 -0400, Robert Haas wrote:
> On Thu, Jun 9, 2016 at 4:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Robert Haas <robertmhaas@gmail.com> writes:
> >> On Sat, May 21, 2016 at 11:45 AM, Tom Lane <tgl@sss.pgh.pa.us> 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.
> >
> >> Tom, there's a patch for this at
> >> https://www.postgresql.org/message-id/574F091A.3050800@proxel.se which
> >> I think you should review, since you were the one who made the tweaks
> >> involved.  Any chance you can do that RSN?
> >
> > I've pushed this with some revisions to make the queries more
> > search-path-safe.  I'm not too happy with the safety of the queries
> > I see already present from the previous patches.  I think stuff
> > like this:
> >
> > UPDATE pg_proc SET proparallel = 's'
> > WHERE oid = 'min(citext)'::regprocedure;
> >
> > needs to be more like
> >
> > UPDATE pg_catalog.pg_proc SET proparallel = 's'
> > WHERE oid = 'min(citext)'::pg_catalog.regprocedure;
> 
> We could do that, but there's no guarantee that "min" or "citext"
> resolve correctly either, is there?  Basically, the search-path-safety
> of many of the scripts already in contrib looks pretty horrendous to
> me.  For example:
> 
> CREATE VIEW pg_buffercache AS
>         SELECT P.* FROM pg_buffercache_pages() AS P
>         (bufferid integer, relfilenode oid, reltablespace oid, reldatabase oid,
>          relforknumber int2, relblocknumber int8, isdirty bool, usagecount int2,
>          pinning_backends int4);
> 
> Well, what guarantee have we that we'll get the right
> pg_buffercache_pages() function?

Aren't both of the above guaranteed due to/* * Set up the search path to contain the target schema, then the schemas *
ofany prerequisite extensions, and nothing else.  In particular this * makes the target schema be the default creation
targetnamespace. * * Note: it might look tempting to use PushOverrideSearchPath for this, * but we cannot do that.  We
haveto actually set the search_path GUC in * case the extension script examines or changes it.  In any case, the *
GUC_ACTION_SAVEmethod is just as convenient. */initStringInfo(&pathbuf);appendStringInfoString(&pathbuf,
quote_identifier(schemaName));foreach(lc,requiredSchemas){    Oid            reqschema = lfirst_oid(lc);    char
*reqname= get_namespace_name(reqschema);
 
    if (reqname)        appendStringInfo(&pathbuf, ", %s", quote_identifier(reqname));}
(void) set_config_option("search_path", pathbuf.data,                         PGC_USERSET, PGC_S_SESSION,
         GUC_ACTION_SAVE, true, 0, false);
 
in extension.c:execute_extension_script()?

> CREATE FUNCTION earth() RETURNS float8
> LANGUAGE SQL IMMUTABLE PARALLEL SAFE
> AS 'SELECT ''6378168''::float8';
> 
> What guarantees we'll get the correct float8 type?
> 
> CREATE FUNCTION sec_to_gc(float8)
> RETURNS float8
> LANGUAGE SQL
> IMMUTABLE STRICT
> PARALLEL SAFE
> AS 'SELECT CASE WHEN $1 < 0 THEN 0::float8 WHEN $1/(2*earth()) > 1
> THEN pi()*earth() ELSE 2*earth()*asin($1/(2*earth())) END';

These aren't though.

Andres

On Tue, Jun 7, 2016 at 11:44 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> More later.

ltree: Skipped per your other email.
ltree_plpython: No point.
pageinspect: Committed.
pg_buffercache: Committed.
pgcrypto: Committed.
pg_freespacemap: Committed.
pg_prewarm: Committed.
pgrowlocks: Committed.

Still more to go.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jun 9, 2016 at 5:45 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2016-06-09 17:40:24 -0400, Robert Haas wrote:
>> On Thu, Jun 9, 2016 at 4:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> > Robert Haas <robertmhaas@gmail.com> writes:
>> >> On Sat, May 21, 2016 at 11:45 AM, Tom Lane <tgl@sss.pgh.pa.us> 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.
>> >
>> >> Tom, there's a patch for this at
>> >> https://www.postgresql.org/message-id/574F091A.3050800@proxel.se which
>> >> I think you should review, since you were the one who made the tweaks
>> >> involved.  Any chance you can do that RSN?
>> >
>> > I've pushed this with some revisions to make the queries more
>> > search-path-safe.  I'm not too happy with the safety of the queries
>> > I see already present from the previous patches.  I think stuff
>> > like this:
>> >
>> > UPDATE pg_proc SET proparallel = 's'
>> > WHERE oid = 'min(citext)'::regprocedure;
>> >
>> > needs to be more like
>> >
>> > UPDATE pg_catalog.pg_proc SET proparallel = 's'
>> > WHERE oid = 'min(citext)'::pg_catalog.regprocedure;
>>
>> We could do that, but there's no guarantee that "min" or "citext"
>> resolve correctly either, is there?  Basically, the search-path-safety
>> of many of the scripts already in contrib looks pretty horrendous to
>> me.  For example:
>>
>> CREATE VIEW pg_buffercache AS
>>         SELECT P.* FROM pg_buffercache_pages() AS P
>>         (bufferid integer, relfilenode oid, reltablespace oid, reldatabase oid,
>>          relforknumber int2, relblocknumber int8, isdirty bool, usagecount int2,
>>          pinning_backends int4);
>>
>> Well, what guarantee have we that we'll get the right
>> pg_buffercache_pages() function?
>
> Aren't both of the above guaranteed due to
>         /*
>          * Set up the search path to contain the target schema, then the schemas
>          * of any prerequisite extensions, and nothing else.  In particular this
>          * makes the target schema be the default creation target namespace.
>          *
>          * Note: it might look tempting to use PushOverrideSearchPath for this,
>          * but we cannot do that.  We have to actually set the search_path GUC in
>          * case the extension script examines or changes it.  In any case, the
>          * GUC_ACTION_SAVE method is just as convenient.
>          */
>         initStringInfo(&pathbuf);
>         appendStringInfoString(&pathbuf, quote_identifier(schemaName));
>         foreach(lc, requiredSchemas)
>         {
>                 Oid                     reqschema = lfirst_oid(lc);
>                 char       *reqname = get_namespace_name(reqschema);
>
>                 if (reqname)
>                         appendStringInfo(&pathbuf, ", %s", quote_identifier(reqname));
>         }
>
>         (void) set_config_option("search_path", pathbuf.data,
>                                                          PGC_USERSET, PGC_S_SESSION,
>                                                          GUC_ACTION_SAVE, true, 0, false);
> in extension.c:execute_extension_script()?

Hmm.  Yeah, that helps.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 06/09/2016 10:48 PM, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Sat, May 21, 2016 at 11:45 AM, Tom Lane <tgl@sss.pgh.pa.us> 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.
>
>> Tom, there's a patch for this at
>> https://www.postgresql.org/message-id/574F091A.3050800@proxel.se which
>> I think you should review, since you were the one who made the tweaks
>> involved.  Any chance you can do that RSN?
>
> I've pushed this with some revisions to make the queries more
> search-path-safe.  I'm not too happy with the safety of the queries
> I see already present from the previous patches.  I think stuff
> like this:
>
> UPDATE pg_proc SET proparallel = 's'
> WHERE oid = 'min(citext)'::regprocedure;
>
> needs to be more like
>
> UPDATE pg_catalog.pg_proc SET proparallel = 's'
> WHERE oid = 'min(citext)'::pg_catalog.regprocedure;

Good point. While I believe that we can trust that ALTER EXTENSION
handles the search path for the functions of the extension we should
qualify things in pg_catalog.

I have attached a patch which adds the shcema, plus an updated patch for
tseach2.

Andreas

On 06/10/2016 01:44 PM, Andreas Karlsson wrote:
> I have attached a patch which adds the shcema, plus an updated patch for
> tseach2.

Forgot adding schema to the tables. Here are new versions.

Andreas

On 06/07/2016 05:54 PM, Andreas Karlsson wrote:
> On 06/07/2016 05:44 PM, Robert Haas wrote:
>> cube: I think we need a new extension version.
>> hstore: Does not apply for me.
>> intarray: Does not apply for me.
>
> Those three and ltree, pg_trgm, and seg depend on my patch with fixes
> for the GiST/GIN function signatures in
> https://www.postgresql.org/message-id/574F091A.3050800@proxel.se. The
> reason for the dependency is that I also mark the those functions with
> changed signatures as parallel safe.

I have rebased all my patches on the current master now (and skipped the
extensions I previously listed).

Andreas

On Tue, Jun 14, 2016 at 6:37 AM, Andreas Karlsson <andreas@proxel.se> wrote:
> I have rebased all my patches on the current master now (and skipped the
> extensions I previously listed).

Thanks, this is really helpful.  It was starting to get hard to keep
track of what hadn't been applied yet.  I decided to prioritize
getting committed the patches where the extension version had already
been bumped by 749a787c5b25ae33b3d4da0ef12aa05214aa73c7, so I've now
committed the patches for cube, hstore, intarray, ltree, pg_trgm, and
seg.  However, in pg_trgm, I changed some of the functions that you
had marked as PARALLEL RESTRICTED to be PARALLEL SAFE, because I
didn't see any reason why they needed to be PARALLEL RESTRICTED.  It's
OK for a parallel-safe function to depend on GUC values, because those
are synchronized from the leader to all worker processes.  Random
global variables won't necessarily be kept in sync, but anything
controlled by the GUC mechanism will be.  If there's some other reason
you think those functions aren't parallel-safe, please let me know.

I'm still in favor of rejecting the adminpack patch.  To me, that just
seems like attaching a larger magazine to the gun pointed at your
foot.  I can't deny that in a strict sense those functions are
parallel-safe, but I think they are better left alone.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jun 14, 2016 at 1:51 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Jun 14, 2016 at 6:37 AM, Andreas Karlsson <andreas@proxel.se> wrote:
>> I have rebased all my patches on the current master now (and skipped the
>> extensions I previously listed).
>
> Thanks, this is really helpful.  It was starting to get hard to keep
> track of what hadn't been applied yet.  I decided to prioritize
> getting committed the patches where the extension version had already
> been bumped by 749a787c5b25ae33b3d4da0ef12aa05214aa73c7, so I've now
> committed the patches for cube, hstore, intarray, ltree, pg_trgm, and
> seg.

I've now also committed the patches for sslinfo, unaccept, uuid-ossp, and xml2.

I took at look at the patch for tsearch2, but I think token_type() is
mismarked.  You have it marked PARALLEL SAFE but seems to depend on
the result of GetCurrentParser(), which returns a backend-private
state variable.  That was the only clear mistake I found, but I tend
to think that changing the markings on anything defined by
UNSUPPORTED_FUNCTION() is pretty silly, because there's no point in
going to extra planner effort to generate a parallel plan only to
error out as soon as we try to execute it.  I think you should leave
all of those out of the patch.

I also took a look at the patch for tablefunc.  I think that you've
got the markings right, here, but I think that it would be good to add
PARALLEL UNSAFE explicitly to the 1.1 version of the file for the
functions are unsafe, and add a comment like "-- query might do
anything" or some other indication as to why they are so marked, for
the benefit of future readers.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 06/14/2016 07:51 PM, Robert Haas wrote:
> On Tue, Jun 14, 2016 at 6:37 AM, Andreas Karlsson <andreas@proxel.se> wrote:
>> I have rebased all my patches on the current master now (and skipped the
>> extensions I previously listed).
>
> Thanks, this is really helpful.  It was starting to get hard to keep
> track of what hadn't been applied yet.  I decided to prioritize
> getting committed the patches where the extension version had already
> been bumped by 749a787c5b25ae33b3d4da0ef12aa05214aa73c7, so I've now
> committed the patches for cube, hstore, intarray, ltree, pg_trgm, and
> seg.  However, in pg_trgm, I changed some of the functions that you
> had marked as PARALLEL RESTRICTED to be PARALLEL SAFE, because I
> didn't see any reason why they needed to be PARALLEL RESTRICTED.  It's
> OK for a parallel-safe function to depend on GUC values, because those
> are synchronized from the leader to all worker processes.  Random
> global variables won't necessarily be kept in sync, but anything
> controlled by the GUC mechanism will be.  If there's some other reason
> you think those functions aren't parallel-safe, please let me know.

Nah, this is a leftover from before I realized that GUCs are safe. I 
thought I went through all the code and fixed this misunderstanding. 
Thanks for spotting this.

> I'm still in favor of rejecting the adminpack patch.  To me, that just
> seems like attaching a larger magazine to the gun pointed at your
> foot.  I can't deny that in a strict sense those functions are
> parallel-safe, but I think they are better left alone.

Making them parallel restricted should prevent them from being a 
footgun, but I also do not see any huge benefit from doing so (while 
making them safe seems dangerous). I do not care either way.

Andreas

On 06/14/2016 09:55 PM, Robert Haas wrote:
> On Tue, Jun 14, 2016 at 1:51 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Tue, Jun 14, 2016 at 6:37 AM, Andreas Karlsson <andreas@proxel.se> wrote:
>>> I have rebased all my patches on the current master now (and skipped the
>>> extensions I previously listed).
>>
>> Thanks, this is really helpful.  It was starting to get hard to keep
>> track of what hadn't been applied yet.  I decided to prioritize
>> getting committed the patches where the extension version had already
>> been bumped by 749a787c5b25ae33b3d4da0ef12aa05214aa73c7, so I've now
>> committed the patches for cube, hstore, intarray, ltree, pg_trgm, and
>> seg.
>
> I've now also committed the patches for sslinfo, unaccept, uuid-ossp, and xml2.

Thanks!

> I took at look at the patch for tsearch2, but I think token_type() is
> mismarked.  You have it marked PARALLEL SAFE but seems to depend on
> the result of GetCurrentParser(), which returns a backend-private
> state variable.

Hm, as far as I can tell that is only for token_type() which I made 
RESTRICTED while token_type(int4) and token_type(text) do not call 
GetCurrentParser().

> That was the only clear mistake I found, but I tend
> to think that changing the markings on anything defined by
> UNSUPPORTED_FUNCTION() is pretty silly, because there's no point in
> going to extra planner effort to generate a parallel plan only to
> error out as soon as we try to execute it.  I think you should leave
> all of those out of the patch.

I will fix this.

> I also took a look at the patch for tablefunc.  I think that you've
> got the markings right, here, but I think that it would be good to add
> PARALLEL UNSAFE explicitly to the 1.1 version of the file for the
> functions are unsafe, and add a comment like "-- query might do
> anything" or some other indication as to why they are so marked, for
> the benefit of future readers.

Good suggestion.

Andreas

On 01-06-2016 20:52, Andreas Karlsson wrote:
> I think at least three of the four aggregate functions are little used,
> so I do not think many users would be affected. And only min(citext) and
> max(citext) can make use of the parallel aggregation.
> 
> The functions are:
> 
> min(citext)
> max(citext)
> int_array_aggregate(int4)
> rewrite(tsquery[])
> 
I don't think those functions are used a lot (as you said) to justify
adding more code in 9.6. Let's not be in a hurry on something that can
wait some months.


--   Euler Taveira                   Timbira - http://www.timbira.com.br/  PostgreSQL: Consultoria, Desenvolvimento,
Suporte24x7 e Treinamento
 

On Thu, Jun 16, 2016 at 9:15 AM, Andreas Karlsson <andreas@proxel.se> wrote:
>> That was the only clear mistake I found, but I tend
>> to think that changing the markings on anything defined by
>> UNSUPPORTED_FUNCTION() is pretty silly, because there's no point in
>> going to extra planner effort to generate a parallel plan only to
>> error out as soon as we try to execute it.  I think you should leave
>> all of those out of the patch.
>
> I will fix this.
>
>> I also took a look at the patch for tablefunc.  I think that you've
>> got the markings right, here, but I think that it would be good to add
>> PARALLEL UNSAFE explicitly to the 1.1 version of the file for the
>> functions are unsafe, and add a comment like "-- query might do
>> anything" or some other indication as to why they are so marked, for
>> the benefit of future readers.
>
> Good suggestion.

I was kind of hoping you'd have a new version of this posted already.
beta2 is wrapping on Monday, and I'm inclined to shelve anything that
isn't done by then for the next release.  And I don't really plan to
work much this weekend.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 06/17/2016 09:11 PM, Robert Haas wrote:
> I was kind of hoping you'd have a new version of this posted already.
> beta2 is wrapping on Monday, and I'm inclined to shelve anything that
> isn't done by then for the next release.  And I don't really plan to
> work much this weekend.

Here they are. Shelve them if you like. They are some of the least
important extensions that still should be fixed so they can end up in 10
if you do not find the time.

Andreas

On Sun, Jun 19, 2016 at 4:21 PM, Andreas Karlsson <andreas@proxel.se> wrote:
> On 06/17/2016 09:11 PM, Robert Haas wrote:
>> I was kind of hoping you'd have a new version of this posted already.
>> beta2 is wrapping on Monday, and I'm inclined to shelve anything that
>> isn't done by then for the next release.  And I don't really plan to
>> work much this weekend.
>
> Here they are. Shelve them if you like. They are some of the least important
> extensions that still should be fixed so they can end up in 10 if you do not
> find the time.

Thanks.  I think it's too late to try to push anything else into
beta2, with less than 24 hours to go before wrap.  I'll just leave
these for 10.0 unless I hear several votes for pushing them into 9.6
post-beta2.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Jun 19, 2016 at 4:21 PM, Andreas Karlsson <andreas@proxel.se> wrote:
>> Here they are. Shelve them if you like. They are some of the least important
>> extensions that still should be fixed so they can end up in 10 if you do not
>> find the time.

> Thanks.  I think it's too late to try to push anything else into
> beta2, with less than 24 hours to go before wrap.  I'll just leave
> these for 10.0 unless I hear several votes for pushing them into 9.6
> post-beta2.

I agree with not pushing them right now, but I think it would be
sensible to push them post-beta2.  There's already one certain
reason for a forced initdb after beta2 [1], so leaving these for
next cycle seems kinda pointless.
        regards, tom lane

[1] https://www.postgresql.org/message-id/30448.1466190874@sss.pgh.pa.us