Re: [PATCHES] Current enums patch

2007-04-02 Thread Tom Lane
Andrew Dunstan <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> Why not put the create-time length test into EnumValuesCreate's loop, >> which has to grovel through the list already? > My idea was to do the error check before calling TypeCreate. If that > doesn't matter I can move it - it just se

Re: [PATCHES] Current enums patch

2007-04-02 Thread Andrew Dunstan
Tom Lane wrote: Andrew Dunstan <[EMAIL PROTECTED]> writes: Working patch attached. If everyone's happy I'll apply it. Why not put the create-time length test into EnumValuesCreate's loop, which has to grovel through the list already? My idea was to do the error check before callin

Re: [PATCHES] Current enums patch

2007-04-02 Thread Tom Lane
Andrew Dunstan <[EMAIL PROTECTED]> writes: > Working patch attached. If everyone's happy I'll apply it. Why not put the create-time length test into EnumValuesCreate's loop, which has to grovel through the list already? I'm wondering why bother with the temp variable in cstring_enum, as opposed t

Re: [PATCHES] Current enums patch

2007-04-02 Thread Andrew Dunstan
Tom Dunstan wrote: Let's just throw the error instead. (I agree that enum_in can just fail with "no such label", but CREATE TYPE ought to give a specific error about it.) Agreed. Andrew, you said you had a mostly-working patch already? Working patch attached. If everyone's happy I'll appl

Re: [PATCHES] Current enums patch

2007-04-02 Thread Tom Dunstan
Tom Lane wrote: But probably making them act like identifiers is not a good idea, because I doubt we want automatic downcasing in enum_in. People wouldn't be happy if they had to write WHERE color = '"Red"' or > something like that to get at a mixed-case enum label. Ah, that's what you had in

Re: [PATCHES] Current enums patch

2007-04-02 Thread Andrew Dunstan
Tom Dunstan wrote: Tom Lane wrote: While all this reasoning is perfectly OK on its own terms, it ignores the precedents of SQL identifier handling. Maybe we should revisit the question of whether the labels are identifiers. Implying that they shouldn't be quoted like text (or should be doubl

Re: [PATCHES] Current enums patch

2007-04-02 Thread Tom Lane
Andrew Dunstan <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> While all this reasoning is perfectly OK on its own terms, it ignores >> the precedents of SQL identifier handling. Maybe we should revisit the >> question of whether the labels are identifiers. > If we do that can we still cache the

Re: [PATCHES] Current enums patch

2007-04-02 Thread Tom Dunstan
Tom Lane wrote: While all this reasoning is perfectly OK on its own terms, it ignores the precedents of SQL identifier handling. Maybe we should revisit the question of whether the labels are identifiers. Implying that they shouldn't be quoted like text (or should be double-quoted if required

Re: [PATCHES] Current enums patch

2007-04-02 Thread Andrew Dunstan
Tom Lane wrote: While all this reasoning is perfectly OK on its own terms, it ignores the precedents of SQL identifier handling. Maybe we should revisit the question of whether the labels are identifiers. If we do that can we still cache the values in the syscache? My impression fr

Re: [PATCHES] Current enums patch

2007-04-02 Thread Tom Lane
Tom Dunstan <[EMAIL PROTECTED]> writes: > I was about to suggest that we just truncate the value in the input > function and look it up on the basis that if it's too long, the lookup > will fail and we can just tell the user that it wasn't a valid value. > But if there's a valid value that has t

Re: [PATCHES] Current enums patch

2007-04-02 Thread Tom Dunstan
Hm, I suppose we should apply truncate_identifier rather than letting the strings be blindly truncated (perhaps in mid-character). Should we have it throw the truncation NOTICE, or not? First thought is to do so during CREATE TYPE but not during plain enum_in(). I don't see much point in tr

Re: [PATCHES] Current enums patch

2007-04-02 Thread Andrew Dunstan
Tom Lane wrote: Heikki Linnakangas <[EMAIL PROTECTED]> writes: There's a little bug: postgres=# CREATE TYPE aenum AS ENUM ('a','b','c'); CREATE TYPE postgres=# CREATE TABLE t (e aenum); CREATE TABLE postgres=# INSERT INTO t VALUES ('f

Re: [PATCHES] Current enums patch

2007-04-02 Thread Tom Lane
Heikki Linnakangas <[EMAIL PROTECTED]> writes: > There's a little bug: > postgres=# CREATE TYPE aenum AS ENUM ('a','b','c'); CREATE TYPE > postgres=# CREATE TABLE t (e aenum); CREATE TABLE postgres=# INSERT > INTO t VALUES > ('fo

Re: [PATCHES] Current enums patch

2007-04-02 Thread Tom Lane
"Tom Dunstan" <[EMAIL PROTECTED]> writes: >> Looks like we need to check the length on type creation >> too. > It works for me (ie fails with an appropriate error) locally > on Linux FC6 x86-64. Perhaps this platform initializes > memory to zero on allocation? Sounds more like you're testing with

Re: [PATCHES] Current enums patch

2007-04-02 Thread Tom Dunstan
> Looks like we need to check the length on type creation > too. > > I'll fix later if not beaten to it. It works for me (ie fails with an appropriate error) locally on Linux FC6 x86-64. Perhaps this platform initializes memory to zero on allocation? I dunno. Anyway, if you can reproduce it, plea

Re: [PATCHES] Current enums patch

2007-04-02 Thread Andrew Dunstan
Heikki Linnakangas wrote: Tom Lane wrote: Tom Dunstan <[EMAIL PROTECTED]> writes: Here's the current version of the enums patch. Applied with revisions --- some cosmetic, some not so much. Attached is the patch-as-applied if you care to compare; feel free to ask questions about anything not o

Re: [PATCHES] Current enums patch

2007-04-02 Thread Heikki Linnakangas
Tom Lane wrote: Tom Dunstan <[EMAIL PROTECTED]> writes: Here's the current version of the enums patch. Applied with revisions --- some cosmetic, some not so much. Attached is the patch-as-applied if you care to compare; feel free to ask questions about anything not obvious. There's a little

Re: [PATCHES] Current enums patch

2007-04-01 Thread Tom Lane
Tom Dunstan <[EMAIL PROTECTED]> writes: > Here's the current version of the enums patch. Applied with revisions --- some cosmetic, some not so much. Attached is the patch-as-applied if you care to compare; feel free to ask questions about anything not obvious. regards, tom

Re: [PATCHES] Current enums patch

2007-04-01 Thread Tom Dunstan
Tom Lane wrote: OK, I give up. :) Why? Because the whole point is that the type has to be known at parse time. Oh, duh. :) I've got it working with get_fn_expr_argtype and it seems fairly reasonable. Cool! Thanks Tom ---(end of broadcast)

Re: [PATCHES] Current enums patch

2007-04-01 Thread Tom Lane
Tom Dunstan <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >>> ... could we >>> have a special rule that would look for e.g. a regtype as the first >>> parameter if the return type is generic and there are no generic parameters? >> >> I thought about that too but don't like it much. The problem

Re: [PATCHES] Current enums patch

2007-04-01 Thread Tom Dunstan
Tom Lane wrote: The null Datum itself obviously doesn't carry that info, but the expression tree does, and there are provisions for letting functions retrieve that info --- see get_fn_expr_rettype and get_fn_expr_argtype. Hmm. I vaguely remember that there was some feeling that the PLs wouldn'

Re: [PATCHES] Current enums patch

2007-04-01 Thread Tom Lane
Tom Dunstan <[EMAIL PROTECTED]> writes: > Andrew Dunstan wrote: >> Tom Lane wrote: >>> enum_first, enum_last: these return ANYENUM but violate the rule that >>> a polymorphic-result function must have a polymorphic input argument >>> from which the parser may deduce the actual output type. >> >> I

Re: [PATCHES] Current enums patch

2007-04-01 Thread Tom Dunstan
text_enum: same problem as above, plus not acceptable to assume that it gets the right enum OID, plus very definitely not acceptable to assume that OID and typmod are the same size, plus I don't like the undocumented kluge in getBaseTypeAndTypmod that is pretending to supply the type OID for some

Re: [PATCHES] Current enums patch

2007-04-01 Thread Tom Dunstan
Andrew Dunstan wrote: Tom Lane wrote: enum_first, enum_last: these return ANYENUM but violate the rule that a polymorphic-result function must have a polymorphic input argument from which the parser may deduce the actual output type. Is this a tragedy when the supplied parameter gives the resu

Re: [PATCHES] Current enums patch

2007-04-01 Thread Andrew Dunstan
Tom Lane wrote: enum_first, enum_last: these return ANYENUM but violate the rule that a polymorphic-result function must have a polymorphic input argument from which the parser may deduce the actual output type. Is this a tragedy when the supplied parameter gives the result type directly?

Re: [PATCHES] Current enums patch

2007-03-31 Thread Tom Lane
BTW, I've got fairly serious problems with a few of the "cuter" parts of the patch: enum_first, enum_last: these return ANYENUM but violate the rule that a polymorphic-result function must have a polymorphic input argument from which the parser may deduce the actual output type. enum_range_all: s

Re: [PATCHES] Current enums patch

2007-03-31 Thread Alvaro Herrera
Tom Lane wrote: > Unless someone objects, I'll change this and also revert to the > enumlabel name that seems to have been used originally (it was still > used in the docs). It seems more readable somehow (I guess it's the > lack of either ascenders or descenders in "enumname"). Wow, I wasn't aw

Re: [PATCHES] Current enums patch

2007-03-31 Thread Tom Dunstan
Tom Lane wrote: Andrew Dunstan <[EMAIL PROTECTED]> writes: If you want to review or test the feature, the attached patch can be used as a replacement for the portion that affects parse_coerce.c, and with this it compiles and passes regression. I think it's correct but it should still be OKed b

Re: [PATCHES] Current enums patch

2007-03-31 Thread Tom Lane
Tom Dunstan <[EMAIL PROTECTED]> writes: > If we ditched the second syscache, we'd want some other way to convert a > type OID and name into the enum value oid efficiently. True. OK, never mind that then. I'll still rename it as per your other comment. regards, tom lan

Re: [PATCHES] Current enums patch

2007-03-31 Thread Tom Dunstan
Tom Lane wrote: > Unless someone objects, I'll change this and also revert to the > enumlabel name that seems to have been used originally (it was still > used in the docs). It seems more readable somehow (I guess it's the > lack of either ascenders or descenders in "enumname"). The name/text th

Re: [PATCHES] Current enums patch

2007-03-31 Thread Tom Dunstan
Hi all! Thanks for reviewing the patch! Tom Lane wrote: Andrew Dunstan <[EMAIL PROTECTED]> writes: Tom Lane wrote: Is there a specific reason for pg_enum.enumname to be type name and not type text? IIRC at one stage Tom wanted to try to make these identifiers, but that was quickly abandoned

Re: [PATCHES] Current enums patch

2007-03-31 Thread Andrew Dunstan
Tom Lane wrote: Andrew Dunstan <[EMAIL PROTECTED]> writes: Tom Lane wrote: Is there a specific reason for pg_enum.enumname to be type name and not type text? IIRC at one stage Tom wanted to try to make these identifiers, but that was quickly abandoned. This might be a hango

Re: [PATCHES] Current enums patch

2007-03-31 Thread Tom Lane
Andrew Dunstan <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> Is there a specific reason for >> pg_enum.enumname to be type name and not type text? > IIRC at one stage Tom wanted to try to make these identifiers, but that > was quickly abandoned. This might be a hangover from that. Actually I

Re: [PATCHES] Current enums patch

2007-03-31 Thread Andrew Dunstan
Tom Lane wrote: Here's the current version of the enums patch. [ sounds of reviewing... ] (What are those? It's a bit hard to imagine you singing "doo di doo doo" a la Homer while reviewing ) Is there a specific reason for pg_enum.enumname to be type name and not type text?

Re: [PATCHES] Current enums patch

2007-03-31 Thread Tom Lane
>>> Here's the current version of the enums patch. [ sounds of reviewing... ] Is there a specific reason for pg_enum.enumname to be type name and not type text? ISTM that type name wastes space (because most labels will probably be a lot shorter than 63 bytes) and at the same time imposes an imp

Re: [PATCHES] Current enums patch

2007-03-31 Thread Tom Lane
Andrew Dunstan <[EMAIL PROTECTED]> writes: > If you want to review or test the feature, the attached patch can be > used as a replacement for the portion that affects parse_coerce.c, and > with this it compiles and passes regression. I think it's correct but it > should still be OKed by at least

Re: [PATCHES] Current enums patch

2007-03-31 Thread Andrew Dunstan
Peter Eisentraut wrote: Am Dienstag, 27. März 2007 03:36 schrieb Tom Dunstan: Here's the current version of the enums patch. Not much change from last time, the only thought-inducing stuff was fixing up some macros that changed with the VARLENA changes, and adding a regression test to do basi

Re: [PATCHES] Current enums patch

2007-03-30 Thread Peter Eisentraut
Am Dienstag, 27. März 2007 03:36 schrieb Tom Dunstan: > Here's the current version of the enums patch. Not much change from last > time, the only thought-inducing stuff was fixing up some macros that > changed with the VARLENA changes, and adding a regression test to do > basic checking of RI behav

Re: [PATCHES] Current enums patch

2007-03-29 Thread Bruce Momjian
Your patch has been added to the PostgreSQL unapplied patches list at: http://momjian.postgresql.org/cgi-bin/pgpatches It will be applied as soon as one of the PostgreSQL committers reviews and approves it. --- To

[PATCHES] Current enums patch

2007-03-26 Thread Tom Dunstan
Hi all Here's the current version of the enums patch. Not much change from last time, the only thought-inducing stuff was fixing up some macros that changed with the VARLENA changes, and adding a regression test to do basic checking of RI behavior, after the discussions that we had recently o