Thread: ltree::text not immutable?

ltree::text not immutable?

From
Joe Van Dyk
Date:
create table t1 (json json);
create index on t1 using btree((json::text));
-- Above works as expected

create extension ltree;
create table t2 (ltree ltree);
create index on t2 using btree((ltree::text));
psql:/tmp/t.sql:8: ERROR:  functions in index expression must be marked
IMMUTABLE

What I'm trying to do is quickly grab the root category from an categories
ltree.

Doing something like
  where root_categories.id = subtree(ltree, 0, 1)
and was trying to make some indexes to make this go faster.

Seems like casting ltree to text and the subtree function should be
immutable?

Re: ltree::text not immutable?

From
Tom Lane
Date:
Joe Van Dyk <joe@tanga.com> writes:
> Seems like casting ltree to text and the subtree function should be
> immutable?

Hm, yeah, I can see no reason why ltree_in and ltree_out shouldn't be
immutable.  They surely ought not be volatile, which is the way they
are marked (by default) right now.  The other types in the ltree
module have the same disease.

More generally, it seems like we ought to have a test in the type_sanity
regression script that checks that type I/O functions aren't volatile,
because there are various embedded assumptions that this is so, cf
commits aab353a60b95aadc00f81da0c6d99bde696c4b75 and
3db6524fe63f0598dcb2b307bb422bc126f2b15d.

That wouldn't have directly caught this problem because we don't apply
type_sanity or opr_sanity to contrib modules, only to core functions.
I have done that manually in the past and think I'll go do it again
to see what else crawls out ... but I wonder if anyone can think of
a way to automate that?

Meanwhile, as far as fixing the immediate issue goes, I propose just
fixing the misdeclarations in ltree--1.0.sql without going through all
the pushups of making an ltree--1.1.sql.  In the past we've fixed
similar errors without a catversion bump on the grounds that the
implications weren't large enough to justify a catversion bump.
I think the equivalent conclusion here is we don't need an extension
version bump.
        regards, tom lane



Re: ltree::text not immutable?

From
Tom Lane
Date:
I wrote:
> More generally, it seems like we ought to have a test in the type_sanity
> regression script that checks that type I/O functions aren't volatile,
> because there are various embedded assumptions that this is so, cf
> commits aab353a60b95aadc00f81da0c6d99bde696c4b75 and
> 3db6524fe63f0598dcb2b307bb422bc126f2b15d.

> That wouldn't have directly caught this problem because we don't apply
> type_sanity or opr_sanity to contrib modules, only to core functions.
> I have done that manually in the past and think I'll go do it again
> to see what else crawls out ... but I wonder if anyone can think of
> a way to automate that?

Actually, the right thing to do if we want to enforce this is for
CREATE TYPE to check the marking.  We'd still need a type_sanity
test to check erroneous declarations of built-in types, but a complaint
in CREATE TYPE would cover all the contrib modules as well as third-party
code.

I wouldn't propose back-patching such an error check, but it seems
reasonable to add it for 9.5.  Any objections?
        regards, tom lane



Re: ltree::text not immutable?

From
Tom Lane
Date:
I wrote:
>> More generally, it seems like we ought to have a test in the type_sanity
>> regression script that checks that type I/O functions aren't volatile,
>> ...

> Actually, the right thing to do if we want to enforce this is for
> CREATE TYPE to check the marking.  We'd still need a type_sanity
> test to check erroneous declarations of built-in types, but a complaint
> in CREATE TYPE would cover all the contrib modules as well as third-party
> code.

> I wouldn't propose back-patching such an error check, but it seems
> reasonable to add it for 9.5.  Any objections?

On the way to fixing this, I was unpleasantly surprised to discover that
we have one I/O function in contrib that *actually is* volatile, and not
just thoughtlessly marked that way.  That's chkpass_in(), which is
volatile because it goes to the trouble of using random() to produce a
new password salt value on every call.

Not entirely sure what to do about this.  It seems like for the purposes
of contrib/chkpass, it's a good thing that chkpass_in() won't reuse the
same salt.  Weak as a 12-bit salt might be nowadays, it's still better
than no salt.  Nonetheless, this behavior is breaking assumptions made
in places like array_in and record_in.

For the moment I'm tempted to mark chkpass_in as stable (with a comment
explaining that it isn't really) just so we can put in the error check
in CREATE TYPE.  But I wonder if anyone has a better idea.
        regards, tom lane



Re: [HACKERS] ltree::text not immutable?

From
Jim Nasby
Date:
On 10/26/14, 3:46 PM, Tom Lane wrote:
> I wrote:
>>> More generally, it seems like we ought to have a test in the type_sanity
>>> regression script that checks that type I/O functions aren't volatile,
>>> ...
>
>> Actually, the right thing to do if we want to enforce this is for
>> CREATE TYPE to check the marking.  We'd still need a type_sanity
>> test to check erroneous declarations of built-in types, but a complaint
>> in CREATE TYPE would cover all the contrib modules as well as third-party
>> code.
>
>> I wouldn't propose back-patching such an error check, but it seems
>> reasonable to add it for 9.5.  Any objections?
>
> On the way to fixing this, I was unpleasantly surprised to discover that
> we have one I/O function in contrib that *actually is* volatile, and not
> just thoughtlessly marked that way.  That's chkpass_in(), which is
> volatile because it goes to the trouble of using random() to produce a
> new password salt value on every call.
>
> Not entirely sure what to do about this.  It seems like for the purposes
> of contrib/chkpass, it's a good thing that chkpass_in() won't reuse the
> same salt.  Weak as a 12-bit salt might be nowadays, it's still better
> than no salt.  Nonetheless, this behavior is breaking assumptions made
> in places like array_in and record_in.
>
> For the moment I'm tempted to mark chkpass_in as stable (with a comment
> explaining that it isn't really) just so we can put in the error check
> in CREATE TYPE.  But I wonder if anyone has a better idea.

The check could explicitly ignore chkpass_in. That's pretty grotty, but perhaps less so than marking it stable when
it'snot...
 
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com



Re: ltree::text not immutable?

From
Alvaro Herrera
Date:
Tom Lane wrote:

> Not entirely sure what to do about this.  It seems like for the purposes
> of contrib/chkpass, it's a good thing that chkpass_in() won't reuse the
> same salt.  Weak as a 12-bit salt might be nowadays, it's still better
> than no salt.  Nonetheless, this behavior is breaking assumptions made
> in places like array_in and record_in.
> 
> For the moment I'm tempted to mark chkpass_in as stable (with a comment
> explaining that it isn't really) just so we can put in the error check
> in CREATE TYPE.  But I wonder if anyone has a better idea.

Can we have a separate function that accepts unencrypted passwords, and
change chkpass_in to only accept encrypted ones?  Similar to
to_tsquery() et al.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services



Re: ltree::text not immutable?

From
Tom Lane
Date:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> Not entirely sure what to do about this.  It seems like for the purposes
>> of contrib/chkpass, it's a good thing that chkpass_in() won't reuse the
>> same salt.  Weak as a 12-bit salt might be nowadays, it's still better
>> than no salt.  Nonetheless, this behavior is breaking assumptions made
>> in places like array_in and record_in.
>> 
>> For the moment I'm tempted to mark chkpass_in as stable (with a comment
>> explaining that it isn't really) just so we can put in the error check
>> in CREATE TYPE.  But I wonder if anyone has a better idea.

> Can we have a separate function that accepts unencrypted passwords, and
> change chkpass_in to only accept encrypted ones?  Similar to
> to_tsquery() et al.

Well, that would undoubtedly have been a better way to design the module
to start with, but we're not working in a green field.  I doubt we can
get away with changing the type's behavior that much.  That is, assuming
there are any users of it at all, which there might not be ;-)

An alternative that just occurred to me is to put the no-volatile-
I/O-functions check into CREATE TYPE, but make it just WARNING not
ERROR.  That would be nearly as good as an ERROR in terms of nudging
people who'd accidentally omitted a volatility marking from their
custom types.  But we could leave chkpass as-is and wait to see if
anyone complains "hey, why am I getting this warning?".  If we don't
hear anyone complaining, maybe that means we can get away with changing
the type's behavior in 9.6 or later.
        regards, tom lane



Re: ltree::text not immutable?

From
Tom Lane
Date:
I wrote:
> An alternative that just occurred to me is to put the no-volatile-
> I/O-functions check into CREATE TYPE, but make it just WARNING not
> ERROR.  That would be nearly as good as an ERROR in terms of nudging
> people who'd accidentally omitted a volatility marking from their
> custom types.  But we could leave chkpass as-is and wait to see if
> anyone complains "hey, why am I getting this warning?".  If we don't
> hear anyone complaining, maybe that means we can get away with changing
> the type's behavior in 9.6 or later.

Attached is a complete patch along these lines.  As I suggested earlier,
this just makes the relevant changes in ltree--1.0.sql and
pg_trgm--1.1.sql without bumping their extension version numbers,
since it doesn't seem important enough to justify a version bump.

I propose that we could back-patch the immutability-additions in ltree and
pg_trgm, since they won't hurt anything and might make life a little
easier for future adopters of those modules.  The WARNING additions should
only go into HEAD though.

            regards, tom lane

diff --git a/contrib/ltree/ltree--1.0.sql b/contrib/ltree/ltree--1.0.sql
index 5a2f375..7d55fc6 100644
*** a/contrib/ltree/ltree--1.0.sql
--- b/contrib/ltree/ltree--1.0.sql
***************
*** 6,17 ****
  CREATE FUNCTION ltree_in(cstring)
  RETURNS ltree
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT;

  CREATE FUNCTION ltree_out(ltree)
  RETURNS cstring
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT;

  CREATE TYPE ltree (
      INTERNALLENGTH = -1,
--- 6,17 ----
  CREATE FUNCTION ltree_in(cstring)
  RETURNS ltree
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT IMMUTABLE;

  CREATE FUNCTION ltree_out(ltree)
  RETURNS cstring
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT IMMUTABLE;

  CREATE TYPE ltree (
      INTERNALLENGTH = -1,
*************** CREATE OPERATOR CLASS ltree_ops
*** 303,314 ****
  CREATE FUNCTION lquery_in(cstring)
  RETURNS lquery
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT;

  CREATE FUNCTION lquery_out(lquery)
  RETURNS cstring
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT;

  CREATE TYPE lquery (
      INTERNALLENGTH = -1,
--- 303,314 ----
  CREATE FUNCTION lquery_in(cstring)
  RETURNS lquery
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT IMMUTABLE;

  CREATE FUNCTION lquery_out(lquery)
  RETURNS cstring
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT IMMUTABLE;

  CREATE TYPE lquery (
      INTERNALLENGTH = -1,
*************** CREATE OPERATOR ^? (
*** 414,425 ****
  CREATE FUNCTION ltxtq_in(cstring)
  RETURNS ltxtquery
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT;

  CREATE FUNCTION ltxtq_out(ltxtquery)
  RETURNS cstring
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT;

  CREATE TYPE ltxtquery (
      INTERNALLENGTH = -1,
--- 414,425 ----
  CREATE FUNCTION ltxtq_in(cstring)
  RETURNS ltxtquery
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT IMMUTABLE;

  CREATE FUNCTION ltxtq_out(ltxtquery)
  RETURNS cstring
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT IMMUTABLE;

  CREATE TYPE ltxtquery (
      INTERNALLENGTH = -1,
*************** CREATE OPERATOR ^@ (
*** 481,492 ****
  CREATE FUNCTION ltree_gist_in(cstring)
  RETURNS ltree_gist
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT;

  CREATE FUNCTION ltree_gist_out(ltree_gist)
  RETURNS cstring
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT;

  CREATE TYPE ltree_gist (
      internallength = -1,
--- 481,492 ----
  CREATE FUNCTION ltree_gist_in(cstring)
  RETURNS ltree_gist
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT IMMUTABLE;

  CREATE FUNCTION ltree_gist_out(ltree_gist)
  RETURNS cstring
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT IMMUTABLE;

  CREATE TYPE ltree_gist (
      internallength = -1,
diff --git a/contrib/pg_trgm/pg_trgm--1.1.sql b/contrib/pg_trgm/pg_trgm--1.1.sql
index 1fff7af..34b37e4 100644
*** a/contrib/pg_trgm/pg_trgm--1.1.sql
--- b/contrib/pg_trgm/pg_trgm--1.1.sql
*************** CREATE OPERATOR <-> (
*** 53,64 ****
  CREATE FUNCTION gtrgm_in(cstring)
  RETURNS gtrgm
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT;

  CREATE FUNCTION gtrgm_out(gtrgm)
  RETURNS cstring
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT;

  CREATE TYPE gtrgm (
          INTERNALLENGTH = -1,
--- 53,64 ----
  CREATE FUNCTION gtrgm_in(cstring)
  RETURNS gtrgm
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT IMMUTABLE;

  CREATE FUNCTION gtrgm_out(gtrgm)
  RETURNS cstring
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT IMMUTABLE;

  CREATE TYPE gtrgm (
          INTERNALLENGTH = -1,
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 55a6881..cbb65f8 100644
*** a/src/backend/commands/typecmds.c
--- b/src/backend/commands/typecmds.c
*************** DefineType(List *names, List *parameters
*** 547,552 ****
--- 547,598 ----
                         NameListToString(analyzeName));
  #endif

+     /*
+      * Print warnings if any of the type's I/O functions are marked volatile.
+      * There is a general assumption that I/O functions are stable or
+      * immutable; this allows us for example to mark record_in/record_out
+      * stable rather than volatile.  Ideally we would throw errors not just
+      * warnings here; but since this check is new as of 9.5, and since the
+      * volatility marking might be just an error-of-omission and not a true
+      * indication of how the function behaves, we'll let it pass as a warning
+      * for now.
+      */
+     if (inputOid && func_volatile(inputOid) == PROVOLATILE_VOLATILE)
+         ereport(WARNING,
+                 (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+                  errmsg("type input function %s should not be volatile",
+                         NameListToString(inputName))));
+     if (outputOid && func_volatile(outputOid) == PROVOLATILE_VOLATILE)
+         ereport(WARNING,
+                 (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+                  errmsg("type output function %s should not be volatile",
+                         NameListToString(outputName))));
+     if (receiveOid && func_volatile(receiveOid) == PROVOLATILE_VOLATILE)
+         ereport(WARNING,
+                 (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+                  errmsg("type receive function %s should not be volatile",
+                         NameListToString(receiveName))));
+     if (sendOid && func_volatile(sendOid) == PROVOLATILE_VOLATILE)
+         ereport(WARNING,
+                 (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+                  errmsg("type send function %s should not be volatile",
+                         NameListToString(sendName))));
+     if (typmodinOid && func_volatile(typmodinOid) == PROVOLATILE_VOLATILE)
+         ereport(WARNING,
+                 (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+                  errmsg("type modifier input function %s should not be volatile",
+                         NameListToString(typmodinName))));
+     if (typmodoutOid && func_volatile(typmodoutOid) == PROVOLATILE_VOLATILE)
+         ereport(WARNING,
+                 (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+                  errmsg("type modifier output function %s should not be volatile",
+                         NameListToString(typmodoutName))));
+
+     /*
+      * OK, we're done checking, time to make the type.  We must assign the
+      * array type OID ahead of calling TypeCreate, since the base type and
+      * array type each refer to the other.
+      */
      array_oid = AssignTypeArrayOid();

      /*
diff --git a/src/test/regress/expected/create_cast.out b/src/test/regress/expected/create_cast.out
index 56cd86e..88f94a6 100644
*** a/src/test/regress/expected/create_cast.out
--- b/src/test/regress/expected/create_cast.out
*************** CREATE TYPE casttesttype;
*** 6,17 ****
  CREATE FUNCTION casttesttype_in(cstring)
     RETURNS casttesttype
     AS 'textin'
!    LANGUAGE internal STRICT;
  NOTICE:  return type casttesttype is only a shell
  CREATE FUNCTION casttesttype_out(casttesttype)
     RETURNS cstring
     AS 'textout'
!    LANGUAGE internal STRICT;
  NOTICE:  argument type casttesttype is only a shell
  CREATE TYPE casttesttype (
     internallength = variable,
--- 6,17 ----
  CREATE FUNCTION casttesttype_in(cstring)
     RETURNS casttesttype
     AS 'textin'
!    LANGUAGE internal STRICT IMMUTABLE;
  NOTICE:  return type casttesttype is only a shell
  CREATE FUNCTION casttesttype_out(casttesttype)
     RETURNS cstring
     AS 'textout'
!    LANGUAGE internal STRICT IMMUTABLE;
  NOTICE:  argument type casttesttype is only a shell
  CREATE TYPE casttesttype (
     internallength = variable,
diff --git a/src/test/regress/expected/create_type.out b/src/test/regress/expected/create_type.out
index 6dfe916..35e8f5d 100644
*** a/src/test/regress/expected/create_type.out
--- b/src/test/regress/expected/create_type.out
*************** CREATE TYPE text_w_default;
*** 41,62 ****
  CREATE FUNCTION int42_in(cstring)
     RETURNS int42
     AS 'int4in'
!    LANGUAGE internal STRICT;
  NOTICE:  return type int42 is only a shell
  CREATE FUNCTION int42_out(int42)
     RETURNS cstring
     AS 'int4out'
!    LANGUAGE internal STRICT;
  NOTICE:  argument type int42 is only a shell
  CREATE FUNCTION text_w_default_in(cstring)
     RETURNS text_w_default
     AS 'textin'
!    LANGUAGE internal STRICT;
  NOTICE:  return type text_w_default is only a shell
  CREATE FUNCTION text_w_default_out(text_w_default)
     RETURNS cstring
     AS 'textout'
!    LANGUAGE internal STRICT;
  NOTICE:  argument type text_w_default is only a shell
  CREATE TYPE int42 (
     internallength = 4,
--- 41,62 ----
  CREATE FUNCTION int42_in(cstring)
     RETURNS int42
     AS 'int4in'
!    LANGUAGE internal STRICT IMMUTABLE;
  NOTICE:  return type int42 is only a shell
  CREATE FUNCTION int42_out(int42)
     RETURNS cstring
     AS 'int4out'
!    LANGUAGE internal STRICT IMMUTABLE;
  NOTICE:  argument type int42 is only a shell
  CREATE FUNCTION text_w_default_in(cstring)
     RETURNS text_w_default
     AS 'textin'
!    LANGUAGE internal STRICT IMMUTABLE;
  NOTICE:  return type text_w_default is only a shell
  CREATE FUNCTION text_w_default_out(text_w_default)
     RETURNS cstring
     AS 'textout'
!    LANGUAGE internal STRICT IMMUTABLE;
  NOTICE:  argument type text_w_default is only a shell
  CREATE TYPE int42 (
     internallength = 4,
diff --git a/src/test/regress/input/create_function_1.source b/src/test/regress/input/create_function_1.source
index 1fded84..f2b1561 100644
*** a/src/test/regress/input/create_function_1.source
--- b/src/test/regress/input/create_function_1.source
***************
*** 5,26 ****
  CREATE FUNCTION widget_in(cstring)
     RETURNS widget
     AS '@libdir@/regress@DLSUFFIX@'
!    LANGUAGE C STRICT;

  CREATE FUNCTION widget_out(widget)
     RETURNS cstring
     AS '@libdir@/regress@DLSUFFIX@'
!    LANGUAGE C STRICT;

  CREATE FUNCTION int44in(cstring)
     RETURNS city_budget
     AS '@libdir@/regress@DLSUFFIX@'
!    LANGUAGE C STRICT;

  CREATE FUNCTION int44out(city_budget)
     RETURNS cstring
     AS '@libdir@/regress@DLSUFFIX@'
!    LANGUAGE C STRICT;

  CREATE FUNCTION check_primary_key ()
      RETURNS trigger
--- 5,26 ----
  CREATE FUNCTION widget_in(cstring)
     RETURNS widget
     AS '@libdir@/regress@DLSUFFIX@'
!    LANGUAGE C STRICT IMMUTABLE;

  CREATE FUNCTION widget_out(widget)
     RETURNS cstring
     AS '@libdir@/regress@DLSUFFIX@'
!    LANGUAGE C STRICT IMMUTABLE;

  CREATE FUNCTION int44in(cstring)
     RETURNS city_budget
     AS '@libdir@/regress@DLSUFFIX@'
!    LANGUAGE C STRICT IMMUTABLE;

  CREATE FUNCTION int44out(city_budget)
     RETURNS cstring
     AS '@libdir@/regress@DLSUFFIX@'
!    LANGUAGE C STRICT IMMUTABLE;

  CREATE FUNCTION check_primary_key ()
      RETURNS trigger
diff --git a/src/test/regress/output/create_function_1.source b/src/test/regress/output/create_function_1.source
index 9926c90..30c2936 100644
*** a/src/test/regress/output/create_function_1.source
--- b/src/test/regress/output/create_function_1.source
***************
*** 4,27 ****
  CREATE FUNCTION widget_in(cstring)
     RETURNS widget
     AS '@libdir@/regress@DLSUFFIX@'
!    LANGUAGE C STRICT;
  NOTICE:  type "widget" is not yet defined
  DETAIL:  Creating a shell type definition.
  CREATE FUNCTION widget_out(widget)
     RETURNS cstring
     AS '@libdir@/regress@DLSUFFIX@'
!    LANGUAGE C STRICT;
  NOTICE:  argument type widget is only a shell
  CREATE FUNCTION int44in(cstring)
     RETURNS city_budget
     AS '@libdir@/regress@DLSUFFIX@'
!    LANGUAGE C STRICT;
  NOTICE:  type "city_budget" is not yet defined
  DETAIL:  Creating a shell type definition.
  CREATE FUNCTION int44out(city_budget)
     RETURNS cstring
     AS '@libdir@/regress@DLSUFFIX@'
!    LANGUAGE C STRICT;
  NOTICE:  argument type city_budget is only a shell
  CREATE FUNCTION check_primary_key ()
      RETURNS trigger
--- 4,27 ----
  CREATE FUNCTION widget_in(cstring)
     RETURNS widget
     AS '@libdir@/regress@DLSUFFIX@'
!    LANGUAGE C STRICT IMMUTABLE;
  NOTICE:  type "widget" is not yet defined
  DETAIL:  Creating a shell type definition.
  CREATE FUNCTION widget_out(widget)
     RETURNS cstring
     AS '@libdir@/regress@DLSUFFIX@'
!    LANGUAGE C STRICT IMMUTABLE;
  NOTICE:  argument type widget is only a shell
  CREATE FUNCTION int44in(cstring)
     RETURNS city_budget
     AS '@libdir@/regress@DLSUFFIX@'
!    LANGUAGE C STRICT IMMUTABLE;
  NOTICE:  type "city_budget" is not yet defined
  DETAIL:  Creating a shell type definition.
  CREATE FUNCTION int44out(city_budget)
     RETURNS cstring
     AS '@libdir@/regress@DLSUFFIX@'
!    LANGUAGE C STRICT IMMUTABLE;
  NOTICE:  argument type city_budget is only a shell
  CREATE FUNCTION check_primary_key ()
      RETURNS trigger
diff --git a/src/test/regress/sql/create_cast.sql b/src/test/regress/sql/create_cast.sql
index ad348da..b11cf88 100644
*** a/src/test/regress/sql/create_cast.sql
--- b/src/test/regress/sql/create_cast.sql
*************** CREATE TYPE casttesttype;
*** 8,18 ****
  CREATE FUNCTION casttesttype_in(cstring)
     RETURNS casttesttype
     AS 'textin'
!    LANGUAGE internal STRICT;
  CREATE FUNCTION casttesttype_out(casttesttype)
     RETURNS cstring
     AS 'textout'
!    LANGUAGE internal STRICT;

  CREATE TYPE casttesttype (
     internallength = variable,
--- 8,18 ----
  CREATE FUNCTION casttesttype_in(cstring)
     RETURNS casttesttype
     AS 'textin'
!    LANGUAGE internal STRICT IMMUTABLE;
  CREATE FUNCTION casttesttype_out(casttesttype)
     RETURNS cstring
     AS 'textout'
!    LANGUAGE internal STRICT IMMUTABLE;

  CREATE TYPE casttesttype (
     internallength = variable,
diff --git a/src/test/regress/sql/create_type.sql b/src/test/regress/sql/create_type.sql
index a4906b6..96a075b 100644
*** a/src/test/regress/sql/create_type.sql
--- b/src/test/regress/sql/create_type.sql
*************** CREATE TYPE text_w_default;
*** 44,62 ****
  CREATE FUNCTION int42_in(cstring)
     RETURNS int42
     AS 'int4in'
!    LANGUAGE internal STRICT;
  CREATE FUNCTION int42_out(int42)
     RETURNS cstring
     AS 'int4out'
!    LANGUAGE internal STRICT;
  CREATE FUNCTION text_w_default_in(cstring)
     RETURNS text_w_default
     AS 'textin'
!    LANGUAGE internal STRICT;
  CREATE FUNCTION text_w_default_out(text_w_default)
     RETURNS cstring
     AS 'textout'
!    LANGUAGE internal STRICT;

  CREATE TYPE int42 (
     internallength = 4,
--- 44,62 ----
  CREATE FUNCTION int42_in(cstring)
     RETURNS int42
     AS 'int4in'
!    LANGUAGE internal STRICT IMMUTABLE;
  CREATE FUNCTION int42_out(int42)
     RETURNS cstring
     AS 'int4out'
!    LANGUAGE internal STRICT IMMUTABLE;
  CREATE FUNCTION text_w_default_in(cstring)
     RETURNS text_w_default
     AS 'textin'
!    LANGUAGE internal STRICT IMMUTABLE;
  CREATE FUNCTION text_w_default_out(text_w_default)
     RETURNS cstring
     AS 'textout'
!    LANGUAGE internal STRICT IMMUTABLE;

  CREATE TYPE int42 (
     internallength = 4,

Re: ltree::text not immutable?

From
Robert Haas
Date:
On Tue, Nov 4, 2014 at 6:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> An alternative that just occurred to me is to put the no-volatile-
>> I/O-functions check into CREATE TYPE, but make it just WARNING not
>> ERROR.  That would be nearly as good as an ERROR in terms of nudging
>> people who'd accidentally omitted a volatility marking from their
>> custom types.  But we could leave chkpass as-is and wait to see if
>> anyone complains "hey, why am I getting this warning?".  If we don't
>> hear anyone complaining, maybe that means we can get away with changing
>> the type's behavior in 9.6 or later.
>
> Attached is a complete patch along these lines.  As I suggested earlier,
> this just makes the relevant changes in ltree--1.0.sql and
> pg_trgm--1.1.sql without bumping their extension version numbers,
> since it doesn't seem important enough to justify a version bump.
>
> I propose that we could back-patch the immutability-additions in ltree and
> pg_trgm, since they won't hurt anything and might make life a little
> easier for future adopters of those modules.  The WARNING additions should
> only go into HEAD though.

I don't understand why you went to all the trouble of building a
versioning system for extensions if you're not going to use it.

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



Re: [HACKERS] ltree::text not immutable?

From
Jim Nasby
Date:
On 11/5/14, 7:38 PM, Robert Haas wrote:
>> Attached is a complete patch along these lines.  As I suggested earlier,
>> >this just makes the relevant changes in ltree--1.0.sql and
>> >pg_trgm--1.1.sql without bumping their extension version numbers,
>> >since it doesn't seem important enough to justify a version bump.
>> >
> I don't understand why you went to all the trouble of building a
> versioning system for extensions if you're not going to use it.

I was about to dismiss this comment since I don't see any real need  for a version bump here, except that AFAIK there's
noway to upgrade an extension without bumping the version, other than resorting to hackery.
 

So I think this does need to be an upgrade. :(
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com



Re: [HACKERS] ltree::text not immutable?

From
Tom Lane
Date:
Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> On 11/5/14, 7:38 PM, Robert Haas wrote:
>> I don't understand why you went to all the trouble of building a
>> versioning system for extensions if you're not going to use it.

> I was about to dismiss this comment since I don't see any real need  for a version bump here, except that AFAIK
there'sno way to upgrade an extension without bumping the version, other than resorting to hackery.
 

Well, the one or two users who actually care can fix the problem with a
manual ALTER FUNCTION.  I'm not sure it's worth making everyone else
jump through update hoops.  If it hadn't taken us years to even notice
the issue, I might be more willing to expend work here, but I really
doubt the audience is larger than one or two people ...
        regards, tom lane



Re: [HACKERS] ltree::text not immutable?

From
Robert Haas
Date:
On Thu, Nov 6, 2014 at 5:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
>> On 11/5/14, 7:38 PM, Robert Haas wrote:
>>> I don't understand why you went to all the trouble of building a
>>> versioning system for extensions if you're not going to use it.
>
>> I was about to dismiss this comment since I don't see any real need  for a version bump here, except that AFAIK
there'sno way to upgrade an extension without bumping the version, other than resorting to hackery.
 
>
> Well, the one or two users who actually care can fix the problem with a
> manual ALTER FUNCTION.  I'm not sure it's worth making everyone else
> jump through update hoops.  If it hadn't taken us years to even notice
> the issue, I might be more willing to expend work here, but I really
> doubt the audience is larger than one or two people ...

I think that's missing the point.  If you bump the version, nobody is
compelled to update.  If you don't, they can't, even if they want to.
This is exactly the opposite of situation from bumping catversion:
bumping catversion forces people to re-initdb, even if they don't care
about picking up the revised catalog contents, but bumping the
extension version does not do that.  It just makes it difficult for
people who want to get the benefit of the changes to do so.

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



Re: [HACKERS] ltree::text not immutable?

From
Christoph Berg
Date:
Re: Tom Lane 2014-11-05 <29132.1415144894@sss.pgh.pa.us>
> I wrote:
> > An alternative that just occurred to me is to put the no-volatile-
> > I/O-functions check into CREATE TYPE, but make it just WARNING not
> > ERROR.  That would be nearly as good as an ERROR in terms of nudging
> > people who'd accidentally omitted a volatility marking from their
> > custom types.  But we could leave chkpass as-is and wait to see if
> > anyone complains "hey, why am I getting this warning?".  If we don't
> > hear anyone complaining, maybe that means we can get away with changing
> > the type's behavior in 9.6 or later.
> 
> Attached is a complete patch along these lines.  As I suggested earlier,
> this just makes the relevant changes in ltree--1.0.sql and
> pg_trgm--1.1.sql without bumping their extension version numbers,
> since it doesn't seem important enough to justify a version bump.
> 
> I propose that we could back-patch the immutability-additions in ltree and
> pg_trgm, since they won't hurt anything and might make life a little
> easier for future adopters of those modules.  The WARNING additions should
> only go into HEAD though.

In HEAD, there's this WARNING now:

WARNING:  type input function chkpass_in should not be volatile

From 66c029c842629958b3ae0d389f24ea3407225723:
   A fly in the ointment is that chkpass_in actually is volatile, because   of its use of random() to generate a fresh
saltwhen presented with a   not-yet-encrypted password.  This is bad because of the general assumption   that I/O
functionsaren't volatile: the consequence is that records or   arrays containing chkpass elements may have input
behaviora bit different   from a bare chkpass column.  But there seems no way to fix this without   breaking existing
usagepatterns for chkpass, and the consequences of the   inconsistency don't seem bad enough to justify that.  So for
themoment,   just document it in a comment.
 

IMHO built-in functions (even if only in contrib) shouldn't be raising
warnings - the user can't do anything about the warnings anyway, so
they shouldn't get notified in the first place.

(Catched by Debian's postgresql-common testsuite)

Christoph
-- 
cb@df7cb.de | http://www.df7cb.de/



Re: [HACKERS] ltree::text not immutable?

From
Tom Lane
Date:
Christoph Berg <cb@df7cb.de> writes:
> In HEAD, there's this WARNING now:
> WARNING:  type input function chkpass_in should not be volatile

Yeah, that's intentional.

> IMHO built-in functions (even if only in contrib) shouldn't be raising
> warnings - the user can't do anything about the warnings anyway, so
> they shouldn't get notified in the first place.

The point is to find out how many people care ...

> (Catched by Debian's postgresql-common testsuite)

... and by "people", I do not mean test suites.
        regards, tom lane



Re: [HACKERS] ltree::text not immutable?

From
Christoph Berg
Date:
Re: Tom Lane 2014-11-17 <6903.1416241481@sss.pgh.pa.us>
> Christoph Berg <cb@df7cb.de> writes:
> > In HEAD, there's this WARNING now:
> > WARNING:  type input function chkpass_in should not be volatile
> 
> Yeah, that's intentional.
> 
> > IMHO built-in functions (even if only in contrib) shouldn't be raising
> > warnings - the user can't do anything about the warnings anyway, so
> > they shouldn't get notified in the first place.
> 
> The point is to find out how many people care ...

Is the point of this to figure out whether to fix this properly, or to
revert to the old code? The current status isn't something that should
be released with 9.5.

> > (Catched by Debian's postgresql-common testsuite)
> 
> ... and by "people", I do not mean test suites.

Well atm this breaks the building of 9.5 packages. This means we are
not going to find out if anyone cares by this way :)

Christoph
-- 
cb@df7cb.de | http://www.df7cb.de/



Re: [HACKERS] ltree::text not immutable?

From
Tom Lane
Date:
Christoph Berg <cb@df7cb.de> writes:
> Re: Tom Lane 2014-11-17 <6903.1416241481@sss.pgh.pa.us>
>> The point is to find out how many people care ...

> Is the point of this to figure out whether to fix this properly, or to
> revert to the old code? The current status isn't something that should
> be released with 9.5.

The warning will not be reverted, if that's what you mean.  The problem
is that chkpass_in is broken by design.  We need to find some actual
users of the type and see what they think is the least painful solution.
(Or, if we find out there aren't any, maybe we'll just flush the whole
module ...)

I will not promise that it will change by 9.5.  It may take some time
to collect information, and 9.4 will not have been in wide use for all
that long before 9.5 freezes.

> Well atm this breaks the building of 9.5 packages. This means we are
> not going to find out if anyone cares by this way :)

You will need to adjust your test.  This is by no means the first time
that we've intentionally shipped contrib modules that generate warnings;
there was that messy business with the => operator ...
        regards, tom lane