Thread: Re: [PERFORM] typoed column name, but postgres didn't grump
[ please continue any further discussion in pgsql-bugs only ] "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes: > Tom Lane <tgl@sss.pgh.pa.us> wrote: >> BTW this seems pretty far off-topic for pgsql-performance. > It is once you understand what's happening. It was probably the 11+ > minutes for the mistyped query run, versus the 28 ms without the > typo, that led them to this list. > I remembered this as an issued that has come up before, but couldn't > come up with good search criteria for finding the old thread before > you posted. If you happen to have a reference or search criteria > for a previous thread, could you post it? Otherwise, a brief > explanation of why this is considered a feature worth keeping would > be good. I know it has been explained before, but it just looks > wrong, on the face of it. What's going on here is an unpleasant interaction of several different features: 1. The notations a.b and b(a) are equivalent: either one can mean the column b of a table a, or an invocation of a function b() that takes a's composite type as parameter. This is an ancient PostQUEL-ism, but we've preserved it because it is helpful for things like emulating computed columns via functions. 2. The notation t(x) will be taken to mean x::t if there's no function t() taking x's type, but there is a cast from x's type to t. This is just as ancient as #1. It doesn't really add any functionality, but I believe we would break a whole lot of users' code if we took it away. Because of #1, this also means that x.t could mean x::t. 3. As of 8.4 or so, there are built-in casts available from pretty much any type (including composites) to all the built-in string types, viz text, varchar, bpchar, name. Upshot is that t.name is a cast to type "name" if there's no column or user-defined function that can match the call. We've seen bug reports on this with respect to both the "name" and "text" cases, though I'm too lazy to trawl the archives for them just now. So, if you want to throw an error for this, you have to choose which of these other things you want to break. I think if I had to pick a proposal, I'd say we should disable #2 for the specific case of casting a composite type to something else. The intentional uses I've seen were all scalar types; and before 8.4 there was no built-in functionality that such a call could match. If we slice off some other part of the functionality, we risk breaking apps that've worked for many years. regards, tom lane
On Fri, Oct 29, 2010 at 3:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > [ please continue any further discussion in pgsql-bugs only ] > > "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes: >> Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> BTW this seems pretty far off-topic for pgsql-performance. > >> It is once you understand what's happening. =A0It was probably the 11+ >> minutes for the mistyped query run, versus the 28 ms without the >> typo, that led them to this list. > >> I remembered this as an issued that has come up before, but couldn't >> come up with good search criteria for finding the old thread before >> you posted. =A0If you happen to have a reference or search criteria >> for a previous thread, could you post it? =A0Otherwise, a brief >> explanation of why this is considered a feature worth keeping would >> be good. =A0I know it has been explained before, but it just looks >> wrong, on the face of it. > > What's going on here is an unpleasant interaction of several different > features: > > 1. The notations a.b and b(a) are equivalent: either one can mean the > column b of a table a, or an invocation of a function b() that takes > a's composite type as parameter. =A0This is an ancient PostQUEL-ism, > but we've preserved it because it is helpful for things like > emulating computed columns via functions. > > 2. The notation t(x) will be taken to mean x::t if there's no function > t() taking x's type, but there is a cast from x's type to t. =A0This is > just as ancient as #1. =A0It doesn't really add any functionality, but > I believe we would break a whole lot of users' code if we took it away. > Because of #1, this also means that x.t could mean x::t. > > 3. As of 8.4 or so, there are built-in casts available from pretty much > any type (including composites) to all the built-in string types, viz > text, varchar, bpchar, name. > > Upshot is that t.name is a cast to type "name" if there's no column or > user-defined function that can match the call. =A0We've seen bug reports > on this with respect to both the "name" and "text" cases, though I'm > too lazy to trawl the archives for them just now. > > So, if you want to throw an error for this, you have to choose which > of these other things you want to break. =A0I think if I had to pick a > proposal, I'd say we should disable #2 for the specific case of casting > a composite type to something else. =A0The intentional uses I've seen were > all scalar types; and before 8.4 there was no built-in functionality > that such a call could match. =A0If we slice off some other part of the > functionality, we risk breaking apps that've worked for many years. Well, then let's do that. It's not the exact fix I'd pick, but it's clearly better than nothing, so I'm willing to sign on to it as a compromise position. --=20 Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> wrote: >> 2. The notation t(x) will be taken to mean x::t if there's no >> function t() taking x's type, but there is a cast from x's type >> to t. >> I think if I had to pick a proposal, I'd say we should disable #2 >> for the specific case of casting a composite type to something >> else. > Well, then let's do that. It's not the exact fix I'd pick, but > it's clearly better than nothing, so I'm willing to sign on to it > as a compromise position. It seems a bad idea to have so many different syntaxes for identical CAST semantics, but there they are, and it's bad to break things. One of the reasons #2 seems like the place to fix it is that it's pretty flaky anyway -- "it will be taken to mean x unless there no y but there is a z" is pretty fragile to start with. Adding one more condition to the places it kicks in doesn't seem as good to me as dropping it entirely, but then I don't have any code which depends on type(value) as a cast syntax -- those who do will likely feel differently. So, I'd rather scrap #2 entirely; but if that really would break much working code, +1 for ignoring it when it would cast a composite to something else. -Kevin
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes: > Robert Haas <robertmhaas@gmail.com> wrote: >>> I think if I had to pick a proposal, I'd say we should disable #2 >>> for the specific case of casting a composite type to something >>> else. >> Well, then let's do that. It's not the exact fix I'd pick, but >> it's clearly better than nothing, so I'm willing to sign on to it >> as a compromise position. > So, I'd rather scrap #2 entirely; but if that really would break > much working code, +1 for ignoring it when it would cast a composite > to something else. Well, assuming for the sake of argument that we have consensus on fixing it like that, is this something we should just do in HEAD, or should we back-patch into 8.4 and 9.0? We'll be hearing about it nigh indefinitely if we don't, but on the other hand this isn't the kind of thing we like to change in released branches. regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes: >> Robert Haas <robertmhaas@gmail.com> wrote: >>>> I think if I had to pick a proposal, I'd say we should disable >>>> #2 for the specific case of casting a composite type to >>>> something else. > >>> Well, then let's do that. It's not the exact fix I'd pick, but >>> it's clearly better than nothing, so I'm willing to sign on to >>> it as a compromise position. > >> So, I'd rather scrap #2 entirely; but if that really would break >> much working code, +1 for ignoring it when it would cast a >> composite to something else. > > Well, assuming for the sake of argument that we have consensus on > fixing it like that, is this something we should just do in HEAD, > or should we back-patch into 8.4 and 9.0? We'll be hearing about > it nigh indefinitely if we don't, but on the other hand this isn't > the kind of thing we like to change in released branches. I can't see back-patching it -- it's a behavior change. On the bright side, in five years after the release where it's removed, it will be out of support. Problem reports caused by it should be tapering off before that.... -Kevin
On Oct 29, 2010, at 4:21 PM, "Kevin Grittner" <Kevin.Grittner@wicourts.gov>= wrote: > Tom Lane <tgl@sss.pgh.pa.us> wrote: >> "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes: >>> Robert Haas <robertmhaas@gmail.com> wrote: >>>>> I think if I had to pick a proposal, I'd say we should disable >>>>> #2 for the specific case of casting a composite type to >>>>> something else. >>=20 >>>> Well, then let's do that. It's not the exact fix I'd pick, but >>>> it's clearly better than nothing, so I'm willing to sign on to >>>> it as a compromise position. >>=20 >>> So, I'd rather scrap #2 entirely; but if that really would break >>> much working code, +1 for ignoring it when it would cast a >>> composite to something else. >>=20 >> Well, assuming for the sake of argument that we have consensus on >> fixing it like that, is this something we should just do in HEAD, >> or should we back-patch into 8.4 and 9.0? We'll be hearing about >> it nigh indefinitely if we don't, but on the other hand this isn't >> the kind of thing we like to change in released branches. >=20 > I can't see back-patching it -- it's a behavior change. >=20 > On the bright side, in five years after the release where it's > removed, it will be out of support. Problem reports caused by it > should be tapering off before that.... Yeah, I think we're going to have to live with it, at least for 8.4. One c= ould make an argument that 9.0 is new enough we could get away with a small= behavior change to avoid a large amount of user confusion. But that may b= e a self-serving argument based on wanting to tamp down the bug reports rat= her than a wisely considered policy decision... so I'm not sure I quite bu= y it. ...Robert
Robert Haas <robertmhaas@gmail.com> writes: > Yeah, I think we're going to have to live with it, at least for 8.4. One could make an argument that 9.0 is new enoughwe could get away with a small behavior change to avoid a large amount of user confusion. But that may be a self-servingargument based on wanting to tamp down the bug reports rather than a wisely considered policy decision... soI'm not sure I quite buy it. Well, tamping down the bug reports is good from the users' point of view too. The argument for not changing it in the back branches is that there might be someone depending on the 8.4/9.0 behavior. However, that seems moderately unlikely. Also, if we wait, that just increases the chances that someone will come to depend on it, and then have a problem when they migrate to 9.1. I think the "risk of breakage" argument has a lot more force when considering long-standing behaviors than things we just recently introduced. regards, tom lane
On Oct 29, 2010, at 5:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> Yeah, I think we're going to have to live with it, at least for 8.4. On= e could make an argument that 9.0 is new enough we could get away with a sm= all behavior change to avoid a large amount of user confusion. But that ma= y be a self-serving argument based on wanting to tamp down the bug reports = rather than a wisely considered policy decision... so I'm not sure I quite= buy it. >=20 > Well, tamping down the bug reports is good from the users' point of view > too. >=20 > The argument for not changing it in the back branches is that there > might be someone depending on the 8.4/9.0 behavior. However, that seems > moderately unlikely. Also, if we wait, that just increases the chances > that someone will come to depend on it, and then have a problem when > they migrate to 9.1. I think the "risk of breakage" argument has a lot > more force when considering long-standing behaviors than things we just > recently introduced. I'm not entirely sure that a behavior we released well over a year ago can = be considered "just recently introduced"... ...Robert
On Fri, Oct 29, 2010 at 2:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > [ please continue any further discussion in pgsql-bugs only ] > > "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes: >> Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> BTW this seems pretty far off-topic for pgsql-performance. > >> It is once you understand what's happening. =C2=A0It was probably the 11+ >> minutes for the mistyped query run, versus the 28 ms without the >> typo, that led them to this list. That is correct. Indeed, at this point, I'm not even sure whether I should have included -performance, here. >> I remembered this as an issued that has come up before, but couldn't >> come up with good search criteria for finding the old thread before >> you posted. =C2=A0If you happen to have a reference or search criteria >> for a previous thread, could you post it? =C2=A0Otherwise, a brief >> explanation of why this is considered a feature worth keeping would >> be good. =C2=A0I know it has been explained before, but it just looks >> wrong, on the face of it. > .. I've spent some time thinking about this. Now, please remember that I'm not a seasoned postgresql veteran like many of you, but I've been doing one kind of programming or another for the better part of 20 years. I am also a strong believer in the principle of least surprise. I say this only so that you might understand better the perspective I'm coming from. With that said, when I read the first part of your first item: > 1. The notations a.b and b(a) are equivalent: either one can mean the > column b of a table a, or an invocation of a function b() that takes > a's composite type as parameter. I feel that, while there may be a fair bit of history here, it's certainly a bit of a surprise. From my perspective, a.b usually means, in most other languages (as it does here), "access the named-thing 'b' from the named-thing 'a' and returns it's value", and whenever parentheses are involved (especially when in the form "b(a)") it means "call function 'b' on named-thing 'a' and return the result". Furthermore, regarding your second point: > 2. The notation t(x) will be taken to mean x::t if there's no function > t() taking x's type, but there is a cast from x's type to t. This is > just as ancient as #1. It doesn't really add any functionality, but > I believe we would break a whole lot of users' code if we took it away. > Because of #1, this also means that x.t could mean x::t. I've always found the form b(a) to have an implicit (if there is a *type* b that can take a thing of type a, then do so (essentially an alternate form of casting). For example, Python and some other languages behave this way. I'm not sure what I might be doing wrong, but there appears to be some sort of inconsistency here, however, as select int(10.1) gives me a syntax error and select 10.1::int does not. So what I'm saying is that for people that do not have a significant background in postgresql that the postquel behavior of treating 'a.b' the same as b(a) is quite a surprise, whereas treating b(a) the same as a::b is not (since frequently "types" are treated like functions in many languages). Therefore, I suggest that you bear these things in mind when discussing or contemplating how the syntax should work - you probably have many more people coming *to* postgresql from other languages than you have users relying on syntax features of postquel. If I saw this behavior ( a.b also meaning b(a) ) in another SQL engine, I would consider it a thoroughly unintuitive wart, however I also understand the need to balance this with existing applications. --=20 Jon
Jon Nelson <jnelson+pgsql@jamponi.net> wrote: > If I saw this behavior ( a.b also meaning b(a) ) in another SQL > engine, I would consider it a thoroughly unintuitive wart I think the main reason it has been kept is the converse -- if you define a function "b" which takes record "a" as its only parameter, you have effectively created a "generated column" on any relation using record type "a". Kind of. It won't show up in the display of the relation's structure or in a SELECT *, and you can't use it in an unqualified reference; but you can use a.b to reference it, which can be convenient. It seems to me that this would be most useful in combination with the inheritance model of PostgreSQL (when used for modeling object hierarchies rather than partitioning). -Kevin
On Tue, Nov 2, 2010 at 4:34 PM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > Jon Nelson <jnelson+pgsql@jamponi.net> wrote: > >> If I saw this behavior ( a.b also meaning b(a) ) in another SQL >> engine, I would consider it a thoroughly unintuitive wart > > I think the main reason it has been kept is the converse -- if you > define a function "b" which takes record "a" as its only parameter, > you have effectively created a "generated column" on any relation > using record type "a". =C2=A0Kind of. =C2=A0It won't show up in the displ= ay of > the relation's structure or in a SELECT *, and you can't use it in > an unqualified reference; but you can use a.b to reference it, which > can be convenient. Aha. I think I understand, now. I also read up on CAST behavior changes between 8.1 and 8.4 (what I'm using), and I found section 34.4.2 "SQL Functions on Composite Types" quite useful. Thanks! --=20 Jon
On Fri, Oct 29, 2010 at 4:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes: >> Robert Haas <robertmhaas@gmail.com> wrote: >>>> I think if I had to pick a proposal, I'd say we should disable #2 >>>> for the specific case of casting a composite type to something >>>> else. > >>> Well, then let's do that. =A0It's not the exact fix I'd pick, but >>> it's clearly better than nothing, so I'm willing to sign on to it >>> as a compromise position. > >> So, I'd rather scrap #2 entirely; but if that really would break >> much working code, +1 for ignoring it when it would cast a composite >> to something else. > > Well, assuming for the sake of argument that we have consensus on fixing > it like that, is this something we should just do in HEAD, or should we > back-patch into 8.4 and 9.0? =A0We'll be hearing about it nigh > indefinitely if we don't, but on the other hand this isn't the kind of > thing we like to change in released branches. Trying to understand real world cases that this would break...would the following now fail w/o explicit cast? create type x as (a int, b int); select f((1,2)); merlin
Merlin Moncure <mmoncure@gmail.com> wrote: > Trying to understand real world cases that this would > break...would the following now fail w/o explicit cast? > > create type x as (a int, b int); > select f((1,2)); It already does: test=# create type x as (a int, b int); CREATE TYPE test=# select f((1,2)); ERROR: function f(record) does not exist LINE 1: select f((1,2)); ^ HINT: No function matches the given name and argument types. You might need to add explicit type casts. -Kevin
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes: > Merlin Moncure <mmoncure@gmail.com> wrote: >> Trying to understand real world cases that this would >> break...would the following now fail w/o explicit cast? >> >> create type x as (a int, b int); >> select f((1,2)); > It already does: I think Merlin probably meant to write "select x((1,2))", but that doesn't work out-of-the-box either. What would be affected is something like select text((1,2)); which you'd now be forced to write as select (1,2)::text; (or you could use CAST notation; but not text(row) or row.text). regards, tom lane
On Thu, Nov 4, 2010 at 12:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes: >> Merlin Moncure <mmoncure@gmail.com> wrote: >>> Trying to understand real world cases that this would >>> break...would the following now fail w/o explicit cast? >>> >>> create type x as (a int, b int); >>> select f((1,2)); > >> It already does: > > I think Merlin probably meant to write "select x((1,2))", but that > doesn't work out-of-the-box either. =A0What would be affected is > something like Actually I didn't -- I left out that there was a function f taking x. I misunderstood your assertion above: "The notation t(x) will be taken to mean x::t if there's no function t() taking x's type, but there is a cast from x's type to t". I thought you meant that it would no longer implicitly cast where it used to for record types, rather than the expression rewrite it was doing (it just clicked). Anyways, no objection to the change, or even the backpatch if you'd like to do that. FWIW. If we ever have an IOCCCish contest for postgresql variant of SQL, there are some real gems in this thread :-). merlin
Tom Lane <tgl@sss.pgh.pa.us> wrote: > What would be affected is something like > > select text((1,2)); > > which you'd now be forced to write as > > select (1,2)::text; > > (or you could use CAST notation; but not text(row) or row.text). Right. As far as I'm aware, there are currently four ways to spell "cast record to text": select cast((1,2) as text); select (1,2)::text; select text((1,2)); select ((1,2)).text; We would be disallowing the last two spellings. They aren't that reliable as casts anyway, since whether they are taken as a cast depends on the field names of the record. test=# create type x as (a int, b int, c text); CREATE TYPE test=# select cast((1,2,'three')::x as text); row ------------- (1,2,three) (1 row) test=# select (1,2,'three')::x::text; row ------------- (1,2,three) (1 row) test=# select text((1,2,'three')::x); text ------------- (1,2,three) (1 row) test=# select ((1,2,'three')::x).text; text ------------- (1,2,three) (1 row) test=# drop type x; DROP TYPE test=# create type x as (a int, b int, text text); CREATE TYPE test=# select cast((1,2,'three')::x as text); row ------------- (1,2,three) (1 row) test=# select (1,2,'three')::x::text; row ------------- (1,2,three) (1 row) test=# select text((1,2,'three')::x); text ------- three (1 row) test=# select ((1,2,'three')::x).text; text ------- three (1 row) So we would only be keeping cast syntax which can be counted on to retain cast semantics in the face of a column name change. -Kevin
Merlin Moncure <mmoncure@gmail.com> writes: > On Thu, Nov 4, 2010 at 12:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes: >>> Merlin Moncure <mmoncure@gmail.com> wrote: >>>> create type x as (a int, b int); >>>> select f((1,2)); >> I think Merlin probably meant to write "select x((1,2))", but that >> doesn't work out-of-the-box either. What would be affected is >> something like > Actually I didn't -- I left out that there was a function f taking x. Ah. No, that would still work after the change. The case that I'm proposing to break is using function-ish notation to invoke a cast from a composite type to some other type whose name you use as if it were a function. Even there, if you've created such a cast following the usual convention of naming the cast function after the target type, it'll still act the same. It's just the built-in I/O-based casts that will stop working this way (for lack of a matching underlying function). regards, tom lane
I wrote: > Ah. No, that would still work after the change. The case that I'm > proposing to break is using function-ish notation to invoke a cast > from a composite type to some other type whose name you use as if it > were a function. Even there, if you've created such a cast following > the usual convention of naming the cast function after the target type, > it'll still act the same. It's just the built-in I/O-based casts that > will stop working this way (for lack of a matching underlying function). Here's a proposed patch, sans documentation as yet. regards, tom lane diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c index b50bce448728280f63c695a688c004bd15bf84cf..9bb100e0c1e83c63b554f65300c03afed563373a 100644 *** a/src/backend/parser/parse_func.c --- b/src/backend/parser/parse_func.c *************** func_get_detail(List *funcname, *** 985,992 **** * can't write "foo[] (something)" as a function call. In theory * someone might want to invoke it as "_foo (something)" but we have * never supported that historically, so we can insist that people ! * write it as a normal cast instead. Lack of historical support is ! * also the reason for not considering composite-type casts here. * * NB: it's important that this code does not exceed what coerce_type * can do, because the caller will try to apply coerce_type if we --- 985,997 ---- * can't write "foo[] (something)" as a function call. In theory * someone might want to invoke it as "_foo (something)" but we have * never supported that historically, so we can insist that people ! * write it as a normal cast instead. ! * ! * We also reject the specific case of COERCEVIAIO for a composite ! * source type and a string-category target type. This is a case that ! * find_coercion_pathway() allows by default, but experience has shown ! * that it's too commonly invoked by mistake. So, again, insist that ! * people use cast syntax if they want to do that. * * NB: it's important that this code does not exceed what coerce_type * can do, because the caller will try to apply coerce_type if we *************** func_get_detail(List *funcname, *** 1017,1024 **** cpathtype = find_coercion_pathway(targetType, sourceType, COERCION_EXPLICIT, &cfuncid); ! iscoercion = (cpathtype == COERCION_PATH_RELABELTYPE || ! cpathtype == COERCION_PATH_COERCEVIAIO); } if (iscoercion) --- 1022,1044 ---- cpathtype = find_coercion_pathway(targetType, sourceType, COERCION_EXPLICIT, &cfuncid); ! switch (cpathtype) ! { ! case COERCION_PATH_RELABELTYPE: ! iscoercion = true; ! break; ! case COERCION_PATH_COERCEVIAIO: ! if ((sourceType == RECORDOID || ! ISCOMPLEX(sourceType)) && ! TypeCategory(targetType) == TYPCATEGORY_STRING) ! iscoercion = false; ! else ! iscoercion = true; ! break; ! default: ! iscoercion = false; ! break; ! } } if (iscoercion)
Tom Lane <tgl@sss.pgh.pa.us> wrote: > Here's a proposed patch, sans documentation as yet. I see you took the surgical approach -- only a cast from a record to a character string type is affected. I agree that will fix the complaints I've seen, and I imagine you're keeping the change narrow to minimize the risk of breaking existing code, but this still looks weird to me: test=# select ('2010-11-05'::date).text; text ------------ 2010-11-05 (1 row) Oh, well -- I guess you have to go well out of your way to shoot your foot with such cases, so that's probably for the best. -Kevin
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes: > Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Here's a proposed patch, sans documentation as yet. > I see you took the surgical approach -- only a cast from a record to > a character string type is affected. I agree that will fix the > complaints I've seen, and I imagine you're keeping the change narrow > to minimize the risk of breaking existing code, but this still looks > weird to me: > test=# select ('2010-11-05'::date).text; > text > ------------ > 2010-11-05 > (1 row) Perhaps, but it's been accepted since 7.3, with few complaints. I think we should only remove the behavior that was added in 8.4. regards, tom lane