unaccent module - two params function should be immutable

On Tue, Feb 19, 2013 at 08:30:29AM +0100, Pavel Stehule wrote:

Hello

There was a proposal to change flag of function to immutable - should
be used in indexes

CREATE FUNCTION unaccent(regdictionary, text)
RETURNS text
AS 'MODULE_PATHNAME', 'unaccent_dict'
LANGUAGE C STABLE STRICT;

is there any progress?

I have developed the attached patch based on your suggestion. I did not
see anything in the code that would make it STABLE, except a lookup of a
dictionary library:

dictOid = get_ts_dict_oid(stringToQualifiedNameList("unaccent"), false);

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

2013/9/11 Bruce Momjian <bruce@momjian.us>

On Tue, Feb 19, 2013 at 08:30:29AM +0100, Pavel Stehule wrote:

Hello

There was a proposal to change flag of function to immutable - should
be used in indexes

CREATE FUNCTION unaccent(regdictionary, text)
RETURNS text
AS 'MODULE_PATHNAME', 'unaccent_dict'
LANGUAGE C STABLE STRICT;

is there any progress?

I have developed the attached patch based on your suggestion. I did not
see anything in the code that would make it STABLE, except a lookup of a
dictionary library:

dictOid = get_ts_dict_oid(stringToQualifiedNameList("unaccent"),
false);

yes, it risk, but similar is with timezones, and other external data.

Regards

Pavel

On Sat, Sep 14, 2013 at 9:42 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I have developed the attached patch based on your suggestion. I did not
see anything in the code that would make it STABLE, except a lookup of a
dictionary library:

dictOid = get_ts_dict_oid(stringToQualifiedNameList("unaccent"),
false);

yes, it risk, but similar is with timezones, and other external data.

That's a catalog lookup - not a reliance on external data.

--
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, Sep 17, 2013 at 10:15:47AM -0400, Robert Haas wrote:

On Sat, Sep 14, 2013 at 9:42 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I have developed the attached patch based on your suggestion. I did not
see anything in the code that would make it STABLE, except a lookup of a
dictionary library:

dictOid = get_ts_dict_oid(stringToQualifiedNameList("unaccent"),
false);

yes, it risk, but similar is with timezones, and other external data.

That's a catalog lookup - not a reliance on external data.

Sorry, I was wrong. Only unaccent_dict() calls get_ts_dict_oid(), and
we aren't changing the signature of that function.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

--
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, Sep 24, 2013 at 05:36:58PM -0400, Bruce Momjian wrote:

On Tue, Sep 17, 2013 at 10:15:47AM -0400, Robert Haas wrote:

On Sat, Sep 14, 2013 at 9:42 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I have developed the attached patch based on your suggestion. I did not
see anything in the code that would make it STABLE, except a lookup of a
dictionary library:

dictOid = get_ts_dict_oid(stringToQualifiedNameList("unaccent"),
false);

yes, it risk, but similar is with timezones, and other external data.

That's a catalog lookup - not a reliance on external data.

Sorry, I was wrong. Only unaccent_dict() calls get_ts_dict_oid(), and
we aren't changing the signature of that function.

Applied.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

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

2013/10/8 Bruce Momjian <bruce@momjian.us>

On Tue, Sep 24, 2013 at 05:36:58PM -0400, Bruce Momjian wrote:

On Tue, Sep 17, 2013 at 10:15:47AM -0400, Robert Haas wrote:

On Sat, Sep 14, 2013 at 9:42 AM, Pavel Stehule <

pavel.stehule@gmail.com> wrote:

I have developed the attached patch based on your suggestion. I

did not

see anything in the code that would make it STABLE, except a lookup

of a

dictionary library:

dictOid =

get_ts_dict_oid(stringToQualifiedNameList("unaccent"),

false);

yes, it risk, but similar is with timezones, and other external data.

That's a catalog lookup - not a reliance on external data.

Sorry, I was wrong. Only unaccent_dict() calls get_ts_dict_oid(), and
we aren't changing the signature of that function.

Applied.

nice

thank you

Regards

Pavel Stehule

On Tue, Oct 8, 2013 at 06:31:03PM +0200, Pavel Stehule wrote:

2013/10/8 Bruce Momjian <bruce@momjian.us>

On Tue, Sep 24, 2013 at 05:36:58PM -0400, Bruce Momjian wrote:

On Tue, Sep 17, 2013 at 10:15:47AM -0400, Robert Haas wrote:

On Sat, Sep 14, 2013 at 9:42 AM, Pavel Stehule <pavel.stehule@gmail.com

wrote:

I have developed the attached patch based on your suggestion. �I did

not

see anything in the code that would make it STABLE, except a lookup

of a

dictionary library:

� � � � dictOid = get_ts_dict_oid(stringToQualifiedNameList

("unaccent"),

false);

yes, it risk, but similar is with timezones, and other external data.

That's a catalog lookup - not a reliance on external data.

Sorry, I was wrong. �Only unaccent_dict() calls get_ts_dict_oid(), and
we aren't changing the signature of that function.

Applied.

nice

thank you

Do we need to update any version or anything? I didn't think so.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

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

2013/10/8 Bruce Momjian <bruce@momjian.us>

On Tue, Oct 8, 2013 at 06:31:03PM +0200, Pavel Stehule wrote:

2013/10/8 Bruce Momjian <bruce@momjian.us>

On Tue, Sep 24, 2013 at 05:36:58PM -0400, Bruce Momjian wrote:

On Tue, Sep 17, 2013 at 10:15:47AM -0400, Robert Haas wrote:

On Sat, Sep 14, 2013 at 9:42 AM, Pavel Stehule <

pavel.stehule@gmail.com

wrote:

I have developed the attached patch based on your suggestion.

I did

not

see anything in the code that would make it STABLE, except a

lookup

of a

dictionary library:

dictOid = get_ts_dict_oid(stringToQualifiedNameList

("unaccent"),

false);

yes, it risk, but similar is with timezones, and other

external data.

That's a catalog lookup - not a reliance on external data.

Sorry, I was wrong. Only unaccent_dict() calls get_ts_dict_oid(),

and

we aren't changing the signature of that function.

Applied.

nice

thank you

Do we need to update any version or anything? I didn't think so.

I am not sure - does pg_upgrade change of flag after upgrade without
increasing version number?

Regards

Pavel

On Tue, Oct 8, 2013 at 06:38:30PM +0200, Pavel Stehule wrote:

I am not sure - does pg_upgrade change of flag after upgrade without increasing
version number?

What happens in pg_upgrade is that the CREATE EXTENSION command is
pg_dump'ed, and run by pg_uprade, and it then pulls from the SQL file to
create the new function signature.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

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

2013/10/8 Bruce Momjian <bruce@momjian.us>

On Tue, Oct 8, 2013 at 06:38:30PM +0200, Pavel Stehule wrote:

I am not sure - does pg_upgrade change of flag after upgrade without

increasing

version number?

What happens in pg_upgrade is that the CREATE EXTENSION command is
pg_dump'ed, and run by pg_uprade, and it then pulls from the SQL file to
create the new function signature.

ok, then it is ok

Bruce Momjian escribi�:

Do we need to update any version or anything? I didn't think so.

I think there should be an 1.1 version here. That way, if somebody is
using the existing definition from the 1.0 module, they can get the new
definition by doing an extension upgrade.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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 Tue, Oct 8, 2013 at 02:25:25PM -0300, Alvaro Herrera wrote:

Bruce Momjian escribi�:

Do we need to update any version or anything? I didn't think so.

I think there should be an 1.1 version here. That way, if somebody is
using the existing definition from the 1.0 module, they can get the new
definition by doing an extension upgrade.

Uh, how would they get this new version? By compiling 9.4 and
installing it in 9.3?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

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

Bruce Momjian escribi�:

On Tue, Oct 8, 2013 at 02:25:25PM -0300, Alvaro Herrera wrote:

Bruce Momjian escribi�:

Do we need to update any version or anything? I didn't think so.

I think there should be an 1.1 version here. That way, if somebody is
using the existing definition from the 1.0 module, they can get the new
definition by doing an extension upgrade.

Uh, how would they get this new version? By compiling 9.4 and
installing it in 9.3?

Oh, is this only in 9.4? Then there's no point.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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

Bruce Momjian <bruce@momjian.us> writes:

[ mark unaccent functions immutable ]

Applied.

This patch is flat out wrong and needs to be reverted.

The functions were correctly marked (by you!) in commit
c0577c92a84cc477a88fe6868c16c4a7e3348b11 on the basis of the discussion of
bug #5781,
/messages/by-id/201012021544.oB2FiTn1041521@wwwmaster.postgresql.org
which was a request exactly like this one and was denied for good and
sufficient reasons. There was absolutely no reasoning given in this
thread that explained why we should ignore the previous objections.

In particular, marking the single-argument version of unaccent() as
immutable is the height of folly because its behavior depends on the
setting of search_path. Changing the two-argument function is maybe
a bit more debatable, but that's not what you did.

If we were going to change the behavior, this patch would still be wrong
because it fails to provide an upgrade path. The objections saying you
needed a 1.1 migration script were completely correct.

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, Nov 8, 2013 at 06:00:53PM -0500, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

[ mark unaccent functions immutable ]

Applied.

This patch is flat out wrong and needs to be reverted.

The functions were correctly marked (by you!) in commit
c0577c92a84cc477a88fe6868c16c4a7e3348b11 on the basis of the discussion of
bug #5781,
/messages/by-id/201012021544.oB2FiTn1041521@wwwmaster.postgresql.org
which was a request exactly like this one and was denied for good and
sufficient reasons. There was absolutely no reasoning given in this
thread that explained why we should ignore the previous objections.

In particular, marking the single-argument version of unaccent() as
immutable is the height of folly because its behavior depends on the
setting of search_path. Changing the two-argument function is maybe
a bit more debatable, but that's not what you did.

If we were going to change the behavior, this patch would still be wrong
because it fails to provide an upgrade path. The objections saying you
needed a 1.1 migration script were completely correct.

Thanks, patch reverted. If people still want this, it needs to be
resbumitted with this feedback in mind.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

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