Re: [BUGS] ltree::text not immutable? - Mailing list pgsql-hackers

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,