Current enums patch

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 majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

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

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/

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

Andrew Dunstan <andrew@dunslane.net> 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

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

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

Andrew Dunstan <andrew@dunslane.net> 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

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> 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

Hi all! Thanks for reviewing the patch!

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> 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

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

Tom Dunstan <pgsql@tomd.cc> 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

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> 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

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 Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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

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

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

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. <Goes to check> 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

Tom Dunstan <pgsql@tomd.cc> 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

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