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

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


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

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

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

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

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

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

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

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

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

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


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

http://archives.postgresql.org

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...8/msg01022.php

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

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

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

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

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