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

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?

Joe Van Dyk <joe(at)tanga(dot)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's not...
--
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(at)2ndquadrant(dot)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

On Tue, Nov 4, 2014 at 6:48 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 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

Jim Nasby <Jim(dot)Nasby(at)BlueTreble(dot)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

On Thu, Nov 6, 2014 at 5:13 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Jim Nasby <Jim(dot)Nasby(at)BlueTreble(dot)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

Re: Tom Lane 2014-11-05 <29132(dot)1415144894(at)sss(dot)pgh(dot)pa(dot)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
--
cb(at)df7cb(dot)de | http://www.df7cb.de/

Christoph Berg <cb(at)df7cb(dot)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(dot)1416241481(at)sss(dot)pgh(dot)pa(dot)us>
> Christoph Berg <cb(at)df7cb(dot)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(at)df7cb(dot)de | http://www.df7cb.de/

Christoph Berg <cb(at)df7cb(dot)de> writes:
> Re: Tom Lane 2014-11-17 <6903(dot)1416241481(at)sss(dot)pgh(dot)pa(dot)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