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 seemed a bit cleaner to do it 
> that way than to have to roll back a change to pg_type.

Well, if this were a performance-critical case then yes, but it isn't.
I'd just as soon keep the code compact.  Besides, typecmds.c isn't
really directly aware of this issue: the reason it's EnumValuesCreate's
bailiwick is that the latter is what's truncating the strings.  If we
ever wanted to change the behavior, it'd be better to keep the
knowledge more localized.

regards, tom lane

---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org


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 calling TypeCreate. If that 
doesn't matter I can move it - it just seemed a bit cleaner to do it 
that way than to have to roll back a change to pg_type.



I'm wondering why bother with the temp variable in cstring_enum,
as opposed to just "if (strlen(name) >= NAMEDATALEN)"?
  


Originally I was going to use the length in the message. But I have 
changed it now.



Also, a comment about why the test is necessary seems appropriate,
since otherwise someone might think it redundant:
/* must check length to prevent Assert failure within SearchSysCache */
  


OK

Lastly, a three-part regression test for this seems a bit silly.
Or a lot silly.  If we added test cases for every niggling little
bug we fix, the regression tests would be taking an hour to run
and would be less productive, not more, because people wouldn't
run them as often.

  


OK.

cheers

andrew

---(end of broadcast)---
TIP 6: explain analyze is your friend


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 to just "if (strlen(name) >= NAMEDATALEN)"?
Also, a comment about why the test is necessary seems appropriate,
since otherwise someone might think it redundant:
/* must check length to prevent Assert failure within SearchSysCache */

Lastly, a three-part regression test for this seems a bit silly.
Or a lot silly.  If we added test cases for every niggling little
bug we fix, the regression tests would be taking an hour to run
and would be less productive, not more, because people wouldn't
run them as often.

regards, tom lane

---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
   subscribe-nomail command to [EMAIL PROTECTED] so that your
   message can get through to the mailing list cleanly


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 apply it.

cheers

andrew

Index: src/backend/commands/typecmds.c
===
RCS file: /cvsroot/pgsql/src/backend/commands/typecmds.c,v
retrieving revision 1.101
diff -c -r1.101 typecmds.c
*** src/backend/commands/typecmds.c	2 Apr 2007 03:49:38 -	1.101
--- src/backend/commands/typecmds.c	2 Apr 2007 19:57:40 -
***
*** 949,954 
--- 949,955 
  	Oid		enumNamespace;
  	Oid		enumTypeOid;
  	AclResult	aclresult;
+ 	ListCell*lc;
  
  	/* Convert list of names to a name and namespace */
  	enumNamespace = QualifiedNameGetCreationNamespace(stmt->typename,
***
*** 970,975 
--- 971,987 
   errmsg("type names must be %d characters or less",
  		NAMEDATALEN - 2)));
  
+ 	foreach (lc, stmt->vals)
+ 	{
+ 		char *lab = strVal(lfirst(lc));
+ 		if (strlen(lab) > (NAMEDATALEN - 1))
+ 			ereport(ERROR,
+ 	(errcode(ERRCODE_INVALID_NAME),
+ 	 errmsg("invalid enum label \"%s\", must be %d characters or less",
+ 			lab,
+ 			NAMEDATALEN - 1)));
+ 	}
+ 
  	/* Create the pg_type entry */
  	enumTypeOid = 
  		TypeCreate(enumName,		/* type name */
Index: src/backend/utils/adt/enum.c
===
RCS file: /cvsroot/pgsql/src/backend/utils/adt/enum.c,v
retrieving revision 1.1
diff -c -r1.1 enum.c
*** src/backend/utils/adt/enum.c	2 Apr 2007 03:49:39 -	1.1
--- src/backend/utils/adt/enum.c	2 Apr 2007 19:57:41 -
***
*** 44,49 
--- 44,57 
  {
  	HeapTuple tup;
  	Oid enumoid;
+ 	size_t namelen = strlen(name);
+ 
+ 	if (namelen >= NAMEDATALEN)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+  errmsg("invalid input value for enum %s: \"%s\"",
+ 		format_type_be(enumtypoid),
+ name)));
  
  	tup = SearchSysCache(ENUMTYPOIDNAME,
  		 ObjectIdGetDatum(enumtypoid),
Index: src/test/regress/expected/enum.out
===
RCS file: /cvsroot/pgsql/src/test/regress/expected/enum.out,v
retrieving revision 1.1
diff -c -r1.1 enum.out
*** src/test/regress/expected/enum.out	2 Apr 2007 03:49:42 -	1.1
--- src/test/regress/expected/enum.out	2 Apr 2007 19:57:43 -
***
*** 40,45 
--- 40,59 
  (6 rows)
  
  --
+ -- Name, Values too long
+ --
+ CREATE TYPE 
+  abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxyz0123456789
+  AS ENUM('a');
+ NOTICE:  identifier "abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxyz0123456789" will be truncated to "abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxy"
+ ERROR:  type names must be 62 characters or less
+ CREATE TYPE toolong AS ENUM 
+ ('abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxyz0123456789');
+ ERROR:  invalid enum label "abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxyz0123456789", must be 63 characters or less
+ INSERT INTO enumtest VALUES
+ ('abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxyz0123456789');
+ ERROR:  invalid input value for enum rainbow: "abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxyz0123456789"
+ --
  -- Operators, no index
  --
  SELECT * FROM enumtest WHERE col = 'orange';
Index: src/test/regress/sql/enum.sql
===
RCS file: /cvsroot/pgsql/src/test/regress/sql/enum.sql,v
retrieving revision 1.1
diff -c -r1.1 enum.sql
*** src/test/regress/sql/enum.sql	2 Apr 2007 03:49:42 -	1.1
--- src/test/regress/sql/enum.sql	2 Apr 2007 19:57:44 -
***
*** 27,32 
--- 27,45 
  SELECT * FROM enumtest;
  
  --
+ -- Name, Values too long
+ --
+ CREATE TYPE 
+  abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxyz0123456789
+  AS ENUM('a');
+ 
+ CREATE TYPE toolong AS ENUM 
+ ('abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxyz0123456789');
+ 
+ INSERT INTO enumtest VALUES
+ ('abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxyz0123456789');
+ 
+ --
  -- Operators, no index
  --
  SELECT * FROM enumtest WHERE col = 'orange';

---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?

   http://www.postgresql.org/docs/faq


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 mind. Yeah, that's a bit verbose.

> 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?

Cheers

Tom


---(end of broadcast)---
TIP 6: explain analyze is your friend


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 
double-quoted if required)? Originally when discussing the patch I 
thought that this was a good idea, but now I'm not so sure. How does 
one set an enum value from e.g. a JDBC PreparedStatement in that 
scenario?



Heh ... I read the statement the other way, i.e. maybe we should treat 
them entirely as strings with no length limitation. The trouble is they 
are half identifiers and half not right now. We certainly can't treat 
them fully as identifiers unless I'm right off course - the ambiguities 
would be horrendous.


cheers

andrew


---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?

  http://www.postgresql.org/docs/faq


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 values in the syscache?

Sure, as long as we're storing them as "name" it's not a problem.

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.  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.)

regards, tom lane

---(end of broadcast)---
TIP 6: explain analyze is your friend


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)? Originally when discussing the patch I 
thought that this was a good idea, but now I'm not so sure. How does one 
set an enum value from e.g. a JDBC PreparedStatement in that scenario?


Cheers

Tom

---(end of broadcast)---
TIP 9: In versions below 8.0, the planner will ignore your desire to
  choose an index scan if your joining column's datatypes do not
  match


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 from what you and TomD said was that it would be at least 
more difficult. If it's possible I'm all in favor.


(Side note: ISTM there is a pretty good case to move the truncation code 
from the lexer to the parser where it can be invoked according to 
context. Maybe something to think about next cycle.).


cheers

andrew

---(end of broadcast)---
TIP 9: In versions below 8.0, the planner will ignore your desire to
  choose an index scan if your joining column's datatypes do not
  match


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 the same 63 bytes as the first 63 
> of the too-long input string, we'll end up looking up the valid one 
> wrongly. So we do need to test for length and die at that point. Can we 
> just fail with the same error message as trying to input a smaller, but 
> similarly invalid string?

> At create time, we should definitely throw an error... if we just 
> truncate the value at byte 64 (with a notice or not) we might truncate 
> in the middle of a multi-byte character. Yuk.

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.

regards, tom lane

---(end of broadcast)---
TIP 9: In versions below 8.0, the planner will ignore your desire to
   choose an index scan if your joining column's datatypes do not
   match


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 truncating.

The patch I have so far gives the regression output shown below (yes, I 
should make the messages more consistent).
 
In fact the truncation and associated NOTICE just strike me as confusing.


[snip]

> + ERROR:  input value too long (74) for enum:
> "abcdefghijklmnopqrsatuvwxyz012345 etc etc

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 the same 63 bytes as the first 63 
of the too-long input string, we'll end up looking up the valid one 
wrongly. So we do need to test for length and die at that point. Can we 
just fail with the same error message as trying to input a smaller, but 
similarly invalid string?


At create time, we should definitely throw an error... if we just 
truncate the value at byte 64 (with a notice or not) we might truncate 
in the middle of a multi-byte character. Yuk.


Cheers

Tom

---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
  subscribe-nomail command to [EMAIL PROTECTED] so that your
  message can get through to the mailing list cleanly


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 
('foo');

server closed the connection unexpectedly



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 truncating.

The patch I have so far gives the regression output shown below (yes, I 
should make the messages more consistent).


In fact the truncation and associated NOTICE just strike me as confusing.

cheers

andrew

+ -- Name, Values too long
+ --
+ CREATE TYPE
+  
abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxyz0123456789

+  AS ENUM('a');
+ NOTICE:  identifier 
"abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxyz0123456789" 
will be truncated to 
"abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxy"

+ ERROR:  type names must be 62 characters or less
+ CREATE TYPE toolong AS ENUM
+ 
('abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxyz0123456789');
+ ERROR:  invalid  enum label 
"abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxyz0123456789":  
must be 63 characters or less

+ INSERT INTO enumtest VALUES
+ 
('abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxyz0123456789');
+ ERROR:  input value too long (74) for enum: 
"abcdefghijklmnopqrsatuvwxyz0123456789abcdefghijklmnopqrsatuvwxyz0123456789"

+ --





---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings


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 
> ('foo');
> server closed the connection unexpectedly

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().

regards, tom lane

---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings


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 without asserts enabled; tut tut.

regards, tom lane

---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
   subscribe-nomail command to [EMAIL PROTECTED] so that your
   message can get through to the mailing list cleanly


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, please have a go, I'm at work and won't be
able to do anything more in depth for some time.

I could have sworn that I had some bounds checking in there
at one point, but it doesn't seem to be in the latest
version of the code. *shrug*. It should be both in
cstring_enum() in enum.c and in DefineEnum() in typecmds.c.

Cheers

Tom

---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org


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 obvious.


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 
('foo'); 


server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded.
postgres=#

In the server log:
TRAP: FailedAssertion("!(keylen < 64)", File: "hashfunc.c", Line: 145)



Looks like we need to check the length on type creation too.

I'll fix later if not beaten to it.

cheers

andrew

---(end of broadcast)---
TIP 4: Have you searched our list archives?

  http://archives.postgresql.org


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 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 
('foo');

server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded.
postgres=#

In the server log:
TRAP: FailedAssertion("!(keylen < 64)", File: "hashfunc.c", Line: 145)


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
  subscribe-nomail command to [EMAIL PROTECTED] so that your
  message can get through to the mailing list cleanly


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 lane



binoqFWzSA6bu.bin
Description: enums-final.patch.gz

---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster


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)---
TIP 3: Have you checked our extensive FAQ?

  http://www.postgresql.org/docs/faq


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 is mainly
>> that it can only work for a constant regtype parameter.

> OK, I give up. :) Why?

Because the whole point is that the type has to be known at parse time.

I've got it working with get_fn_expr_argtype and it seems fairly
reasonable.

regards, tom lane

---(end of broadcast)---
TIP 6: explain analyze is your friend


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't always fill out the FmgrInfo struct, but perhaps that was just 
the case with I/O functions.


... 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 is mainly
that it can only work for a constant regtype parameter.


OK, I give up. :) Why?

Thanks

Tom

---(end of broadcast)---
TIP 6: explain analyze is your friend


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.
>> 
>> Is this a tragedy when the supplied parameter gives the result type 
>> directly?

> I've been having a play with this. If you create a function taking an 
> enum type as a parameter, you can feed the output from enum_first into 
> it, regardless of the type given to enum_first. I doubt that it would be 
> a problem in practice, but it's certainly not great.

Well, that's exactly the point: the proposed functions break the type
system by allowing values of one enum type to be fed to a function
expecting a different one.  Even though that's unlikely to cause a
system crash, it's still wrong.  We need to define these functions in a
way that allows type safety to be checked at parse time, the same as
every other type is required to be.

>>> One rather ugly possibility is that the argument is a
>>> value of the target type --- which you can always have as null, so
>>> 
>>> enum_first(null::rainbow) = 'red'
>>> 
>>> where enum_first is declared as taking and returning anyenum.

> I don't think that'll fly. If it's passed a null value, how does 
> enum_first know which enum type it's dealing with? Can you get the type 
> from the null value in some way?

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.

It occurred to me this morning that get_fn_expr_rettype might
represent salvation for text_enum as well, though I've not tried
it yet.

> ... 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 is mainly
that it can only work for a constant regtype parameter.

regards, tom lane

---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
   subscribe-nomail command to [EMAIL PROTECTED] so that your
   message can get through to the mailing list cleanly


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 small fraction of possible invocation cases.

I think text_enum is probably toast.


This was the ugliest part of the patch, and I mentioned it explicitly in 
the original submission because I was uncomfortable about it. The proper 
solution seemed to be to have another allowed cast function signature 
that would take the regtype rather than the typmod. That got just a 
little beyond what I wanted to do in the patch, and ugly as the 
getBaseTypeAndTypmod hack was, it seemed less intrusive.


Another way to skirt the issue was to simply set the typmod of enum 
types to have their own OID, but a) I wasn't sure what other things the 
system might be inferring from the presence of a typmod, and b) you just 
stated that we can't assume that typmod is big enough to hold an OID anyway.


I'm less concerned about the generic return type in this case, since the 
parser should know the return type when an explicit cast has been 
called.  Hmm, ok, maybe not. Looks like it's using the 
return type of the cast function and throwing away the explicit cast 
information. That's not very nice, but can probably be fixed in the parser.



Thoughts?

Tom

---(end of broadcast)---
TIP 9: In versions below 8.0, the planner will ignore your desire to
  choose an index scan if your joining column's datatypes do not
  match


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 result type 
directly?


I've been having a play with this. If you create a function taking an 
enum type as a parameter, you can feed the output from enum_first into 
it, regardless of the type given to enum_first. I doubt that it would be 
a problem in practice, but it's certainly not great.


> One rather ugly possibility is that the argument is a
> value of the target type --- which you can always have as null, so
>
>enum_first(null::rainbow) = 'red'
>
> where enum_first is declared as taking and returning anyenum.

I don't think that'll fly. If it's passed a null value, how does 
enum_first know which enum type it's dealing with? Can you get the type 
from the null value in some way?


> This
> seems a bit yucky as opposed to the regtype approach in the patch,
> and yet there are cases where it would be more handy --- eg, if
> you are working with a table column "col" of some enum type, you
> can do enum_first(col) instead of hardwiring the enum name.

That's ok, as long as nulls work.

> There might be other better ways, though.  Thoughts?

*Ponder*. In java, you can tie a generic return value to a particular 
class by passing the class in as a bound generic parameter... 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?



As a side note, the helper functions were intended for people writing 
functions in plpgsql or whatever, allowing them to not hardcode the 
values of the enum in their function. I consider them nice-to-have 
rather than definitely required. If we can't come up with a nice way to 
do them for 8.3, I'm not absolutely wedded to them. It would be *nice*, 
though.


I really would like the cast from text, though, but I'll deal with that 
in another email.


Regards

Tom

---(end of broadcast)---
TIP 4: Have you searched our list archives?

  http://archives.postgresql.org


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?


If it really is, maybe we should return text instead of the enum 
directly (or array of text in the case of enum_range).




cheers

andrew



---(end of broadcast)---
TIP 6: explain analyze is your friend


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: same problem except ANYARRAY result.

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 small fraction of possible invocation cases.

I think text_enum is probably toast.  We might be able to salvage the
other three if we can think of some reasonable approach to type
determination.  One rather ugly possibility is that the argument is a
value of the target type --- which you can always have as null, so

enum_first(null::rainbow) = 'red'

where enum_first is declared as taking and returning anyenum.  This
seems a bit yucky as opposed to the regtype approach in the patch,
and yet there are cases where it would be more handy --- eg, if
you are working with a table column "col" of some enum type, you
can do enum_first(col) instead of hardwiring the enum name.

There might be other better ways, though.  Thoughts?

regards, tom lane

---(end of broadcast)---
TIP 9: In versions below 8.0, the planner will ignore your desire to
   choose an index scan if your joining column's datatypes do not
   match


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 aware we were picking our terms based on typography.  It
is kind of cool.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings


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 by at least one Tom. :-)


> Barring objection from Tom D, I'll start with this version.

OK, I've now had a chance to look at Andrew's update of the patch, which 
just seems to pass through the new arrayCoerce parameter to the 
find_coercion_pathway calls. It almost doesn't matter what gets passed 
in: find_coercion_pathway should never set that to true in our calls to 
it in the enum code, as we're passing ANYENUMOID through to the recursed 
call and that'll never hit the array coercion branch.


In summary: looks good to me!

Cheers

Tom

---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?

  http://www.postgresql.org/docs/faq


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 lane

---(end of broadcast)---
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate


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 thing is discussed downthread. I actually started out 
calling the field the name and changed it to the label, but perhaps I 
only did that in the docs. It was probably while I was writing the docs 
that I realized that name could refer to the enum type name or the value 
name, which was confusing, but "value name" was kinda cumbersome, hence 
"label". Change it over with my blessing.


Cheers

Tom

---(end of broadcast)---
TIP 7: You can help support the PostgreSQL project by donating at

   http://www.postgresql.org/about/donate


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. This might be a hangover from that.


Actually I think I see the reason: it's a bit of a pain in the neck to
use the syscache mechanism with text-type lookup keys.  I'm not 100%
convinced that we really need to have syscaches on pg_enum, but if those
stay then it's probably not worth the trouble to avoid the limitation.


Yeah, that was the reason IIRC. The syscaches are used by the I/O 
functions: The one keyed on the pg_enum OID is for output, and the one 
keyed on the type OID and label, err, name, are for input. As suggested 
by a certain party here [1]. There didn't seem to be any text-like key 
types to use in the syscache except the name type, and I didn't see the 
63 byte limit being a big deal, that's way bigger than any sane enum 
name that I've ever seen.


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. I originally 
suggested having a cache that got hooked onto an fn_extra field; that 
idea could be resurrected if you don't like the syscache.


Cheers

Tom


1[] http://archives.postgresql.org/pgsql-hackers/2006-08/msg01022.php

---(end of broadcast)---
TIP 6: explain analyze is your friend


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 hangover from that.



Actually I think I see the reason: it's a bit of a pain in the neck to
use the syscache mechanism with text-type lookup keys.  I'm not 100%
convinced that we really need to have syscaches on pg_enum, but if those
stay then it's probably not worth the trouble to avoid the limitation.


  


That rings a faint bell.

If we don't have syscaches on pg_enum won't enum i/o get more expensive?

cheers

andrew


---(end of broadcast)---
TIP 4: Have you searched our list archives?

  http://archives.postgresql.org


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 think I see the reason: it's a bit of a pain in the neck to
use the syscache mechanism with text-type lookup keys.  I'm not 100%
convinced that we really need to have syscaches on pg_enum, but if those
stay then it's probably not worth the trouble to avoid the limitation.

regards, tom lane

---(end of broadcast)---
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate


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?  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 implementation restriction that
we don't need to have.  It would make sense if the enum labels were
treated syntactically as SQL identifiers, but they're treated as
strings.  And there's no particular win to be had by having a
fixed-length struct, since there's no more fields anyway.
  


IIRC at one stage Tom wanted to try to make these identifiers, but that 
was quickly abandoned. This might be a hangover from that. If someone 
wants to use an insanely long enum label I guess that's their lookout.




cheers

andrew

---(end of broadcast)---
TIP 4: Have you searched our list archives?

  http://archives.postgresql.org


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 implementation restriction that
we don't need to have.  It would make sense if the enum labels were
treated syntactically as SQL identifiers, but they're treated as
strings.  And there's no particular win to be had by having a
fixed-length struct, since there's no more fields anyway.

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").

regards, tom lane

---(end of broadcast)---
TIP 9: In versions below 8.0, the planner will ignore your desire to
   choose an index scan if your joining column's datatypes do not
   match


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 one Tom. :-)

Since Neil seems to have dropped the ball on this, I'm going to pick it
up and review/apply it.  I need a break from thinking about HOT and
other bizarre schemes ;-)

Barring objection from Tom D, I'll start with this version.

regards, tom lane

---(end of broadcast)---
TIP 6: explain analyze is your friend


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
basic checking of RI behavior, after the discussions that we had
recently on the ri_trigger stuff with generic types. The actual behavior
was fixed by Tom's earlier patch, so this is just a sanity check.



Your patch doesn't compile anymore.

ccache cc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Winline 
-Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -g -I. 
-I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2   -c -o 
parse_coerce.o parse_coerce.c -MMD -MP -MF .deps/parse_coerce.Po
parse_coerce.c: In function 'can_coerce_type':
parse_coerce.c:460: error: too few arguments to function 'find_coercion_pathway'
parse_coerce.c: In function 'find_coercion_pathway':
parse_coerce.c:1817: error: too few arguments to function 
'find_coercion_pathway'
parse_coerce.c:1822: error: too few arguments to function 
'find_coercion_pathway'

This was only changed a few days ago, so you need to update your patch.

  


Peter,

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 one Tom. :-)


cheers

andrew
Index: parse_coerce.c
===
RCS file: /cvsroot/pgsql/src/backend/parser/parse_coerce.c,v
retrieving revision 2.152
diff -c -r2.152 parse_coerce.c
*** parse_coerce.c	27 Mar 2007 23:21:10 -	2.152
--- parse_coerce.c	31 Mar 2007 18:53:08 -
***
*** 132,137 
--- 132,138 
  	}
  	if (targetTypeId == ANYOID ||
  		targetTypeId == ANYELEMENTOID ||
+ 		targetTypeId == ANYENUMOID ||
  		(targetTypeId == ANYARRAYOID && inputTypeId != UNKNOWNOID))
  	{
  		/*
***
*** 406,414 
  		if (targetTypeId == ANYOID)
  			continue;
  
! 		/* accept if target is ANYARRAY or ANYELEMENT, for now */
  		if (targetTypeId == ANYARRAYOID ||
! 			targetTypeId == ANYELEMENTOID)
  		{
  			have_generics = true;		/* do more checking later */
  			continue;
--- 407,416 
  		if (targetTypeId == ANYOID)
  			continue;
  
! 		/* accept if target is ANYARRAY, ANYELEMENT or ANYENUM, for now */
  		if (targetTypeId == ANYARRAYOID ||
! 			targetTypeId == ANYELEMENTOID ||
! 			targetTypeId == ANYENUMOID)
  		{
  			have_generics = true;		/* do more checking later */
  			continue;
***
*** 451,456 
--- 453,466 
  			continue;
  
  		/*
+ 		 * If input is an enum, can ANYENUM be cast to target?
+ 		 */
+ 		if (is_type_enum(inputTypeId) &&
+ 			find_coercion_pathway(targetTypeId, ANYENUMOID,
+   ccontext, &funcId, &arrayCoerce))
+ 			continue;
+ 
+ 		/*
  		 * Else, cannot coerce at this argument position
  		 */
  		return false;
***
*** 1070,1076 
  	Oid			array_typeid = InvalidOid;
  	Oid			array_typelem;
  	bool		have_anyelement = false;
! 
  	/*
  	 * Loop through the arguments to see if we have any that are ANYARRAY or
  	 * ANYELEMENT. If so, require the actual types to be self-consistent
--- 1080,1086 
  	Oid			array_typeid = InvalidOid;
  	Oid			array_typelem;
  	bool		have_anyelement = false;
! 	bool		have_enum = false;
  	/*
  	 * Loop through the arguments to see if we have any that are ANYARRAY or
  	 * ANYELEMENT. If so, require the actual types to be self-consistent
***
*** 1079,1085 
  	{
  		Oid			actual_type = actual_arg_types[j];
  
! 		if (declared_arg_types[j] == ANYELEMENTOID)
  		{
  			have_anyelement = true;
  			if (actual_type == UNKNOWNOID)
--- 1089,1096 
  	{
  		Oid			actual_type = actual_arg_types[j];
  
! 		if (declared_arg_types[j] == ANYELEMENTOID ||
! 			declared_arg_types[j] == ANYENUMOID)
  		{
  			have_anyelement = true;
  			if (actual_type == UNKNOWNOID)
***
*** 1087,1092 
--- 1098,1105 
  			if (OidIsValid(elem_typeid) && actual_type != elem_typeid)
  return false;
  			elem_typeid = actual_type;
+ 			if (declared_arg_types[j] == ANYENUMOID)
+ have_enum = true;
  		}
  		else if (declared_arg_types[j] == ANYARRAYOID)
  		{
***
*** 1127,1132 
--- 1140,1151 
  		}
  	}
  
+ 	if (have_enum)
+ 	{
+ 		/* is the given type as enum type? */
+ 		return is_type_enum(elem_typeid);
+ 	}
+ 
  	/* Looks valid */
  	return true;
  }
***
*** 1180,1186 
  	Oid			elem_typeid = InvalidOid;
  	Oid			array_typeid = InvalidOid;
  	Oid			array_typelem;
! 	bool		have_anyelement = (rettype == ANYELEMENTOID);
  
  	/*
  	 * Loop through the arguments to see if we have any that are ANYARRAY or
--- 1199,1206 
  	Oid			elem_typeid = InvalidOid;
  	Oid			array_typeid = InvalidOid;
  	Oid			array_

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 behavior, after the discussions that we had
> recently on the ri_trigger stuff with generic types. The actual behavior
> was fixed by Tom's earlier patch, so this is just a sanity check.

Your patch doesn't compile anymore.

ccache cc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Winline 
-Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -g -I. 
-I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2   -c -o 
parse_coerce.o parse_coerce.c -MMD -MP -MF .deps/parse_coerce.Po
parse_coerce.c: In function 'can_coerce_type':
parse_coerce.c:460: error: too few arguments to function 'find_coercion_pathway'
parse_coerce.c: In function 'find_coercion_pathway':
parse_coerce.c:1817: error: too few arguments to function 
'find_coercion_pathway'
parse_coerce.c:1822: error: too few arguments to function 
'find_coercion_pathway'

This was only changed a few days ago, so you need to update your patch.

-- 
Peter Eisentraut
http://developer.postgresql.org/~petere/

---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org


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.

---


Tom Dunstan wrote:
> 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 on the ri_trigger stuff with generic types. The actual behavior 
> was fixed by Tom's earlier patch, so this is just a sanity check.
> 
> Cheers
> 
> Tom

[ application/x-gzip is not supported, skipping... ]

> 
> ---(end of broadcast)---
> TIP 1: if posting/reading through Usenet, please send an appropriate
>subscribe-nomail command to [EMAIL PROTECTED] so that your
>message can get through to the mailing list cleanly

-- 
  Bruce Momjian  <[EMAIL PROTECTED]>  http://momjian.us
  EnterpriseDB   http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?

   http://www.postgresql.org/docs/faq


[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 on the ri_trigger stuff with generic types. The actual behavior 
was fixed by Tom's earlier patch, so this is just a sanity check.


Cheers

Tom


enums.patch.gz
Description: GNU Zip compressed data

---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
   subscribe-nomail command to [EMAIL PROTECTED] so that your
   message can get through to the mailing list cleanly