Thread: Re: [BUGS] ltree::text not immutable?
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
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
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
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
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
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
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,
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
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
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
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: 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/
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: 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/
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