Re: unaccent module - two params function should be immutable

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?

Regards

Pavel Stehule

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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

2013/9/11 Bruce Momjian <bruce(at)momjian(dot)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

>
> --
> Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
> EnterpriseDB http://enterprisedb.com
>
> + It's impossible for everything to be true. +
>

On Sat, Sep 14, 2013 at 9:42 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

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(dot)stehule(at)gmail(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

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(dot)stehule(at)gmail(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

2013/10/8 Bruce Momjian <bruce(at)momjian(dot)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(dot)stehule(at)gmail(dot)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

>
> --
> Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
> EnterpriseDB http://enterprisedb.com
>
> + It's impossible for everything to be true. +
>

On Tue, Oct 8, 2013 at 06:31:03PM +0200, Pavel Stehule wrote:
>
>
>
> 2013/10/8 Bruce Momjian <bruce(at)momjian(dot)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(dot)stehule(at)gmail(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

2013/10/8 Bruce Momjian <bruce(at)momjian(dot)us>

> On Tue, Oct 8, 2013 at 06:31:03PM +0200, Pavel Stehule wrote:
> >
> >
> >
> > 2013/10/8 Bruce Momjian <bruce(at)momjian(dot)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(dot)stehule(at)gmail(dot)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

>
> --
> Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
> EnterpriseDB http://enterprisedb.com
>
> + Everyone has their own god. +
>

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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

2013/10/8 Bruce Momjian <bruce(at)momjian(dot)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 <bruce(at)momjian(dot)us> http://momjian.us
> EnterpriseDB http://enterprisedb.com
>
> + Everyone has their own god. +
>

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

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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

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

Bruce Momjian <bruce(at)momjian(dot)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,
http://www.postgresql.org/message-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

On Fri, Nov 8, 2013 at 06:00:53PM -0500, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)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,
> http://www.postgresql.org/message-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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +