Re: [HACKERS] [BUGS] ltree::text not immutable?
Re: Tom Lane 2014-11-05 29132.1415144...@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 salt when presented with a not-yet-encrypted password. This is bad because of the general assumption that I/O functions aren't volatile: the consequence is that records or arrays containing chkpass elements may have input behavior a bit different from a bare chkpass column. But there seems no way to fix this without breaking existing usage patterns for chkpass, and the consequences of the inconsistency don't seem bad enough to justify that. So for the moment, 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 -- c...@df7cb.de | http://www.df7cb.de/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] ltree::text not immutable?
Christoph Berg c...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] ltree::text not immutable?
Re: Tom Lane 2014-11-17 6903.1416241...@sss.pgh.pa.us Christoph Berg c...@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 -- c...@df7cb.de | http://www.df7cb.de/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] ltree::text not immutable?
Christoph Berg c...@df7cb.de writes: Re: Tom Lane 2014-11-17 6903.1416241...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] ltree::text not immutable?
On Thu, Nov 6, 2014 at 5:13 PM, Tom Lane t...@sss.pgh.pa.us wrote: Jim Nasby jim.na...@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's no 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] ltree::text not immutable?
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 no way 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] ltree::text not immutable?
Jim Nasby jim.na...@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's no 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] ltree::text not immutable?
On Tue, Nov 4, 2014 at 6:48 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] ltree::text not immutable?
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
Re: [HACKERS] [BUGS] ltree::text not immutable?
Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] ltree::text not immutable?
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] ltree::text not immutable?
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] ltree::text not immutable?
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's not... -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] ltree::text not immutable?
Joe Van Dyk j...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] ltree::text not immutable?
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
