Re: updated WIP: arrays of composites

Attached is my rework of David Fetter's array of composites patch. It
has all the agreed modifications and checks, except altering the name
mangling.

If people prefer I can apply this as it stands and then get working on
the name mangling piece. Or I can hold off.

I'm still wondering if we can get away without a catalog change on that
- e.g. could we look up an array type by looking for a pg_type entry
containing the base type's oid in the typelem field? Or would that be
too slow?

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> I'm still wondering if we can get away without a catalog change on that
> - e.g. could we look up an array type by looking for a pg_type entry
> containing the base type's oid in the typelem field? Or would that be
> too slow?

(a) too slow, (b) not unique.

regards, tom lane

I wrote:
>
> Attached is my rework of David Fetter's array of composites patch. It
> has all the agreed modifications and checks, except altering the name
> mangling.
>
>

Here is the latest with the name mangling piece done - it's got quite
large with the catalog rewrite :-)

It could do with some extra eyeballs - especially the error codes.

I'm not sure if we need to adjust the test in pg_dump.c - maybe we
should look at using the same test as the backend (typlen == -1) to
disambiguate real arrays from subscriptable base types.

cheers

andrew

>>
>> Attached is my rework of David Fetter's array of composites patch. It
>> has all the agreed modifications and checks, except altering the name
>> mangling.
>>
>>
>
> Here is the latest with the name mangling piece done - it's got quite
> large with the catalog rewrite :-)
>
> It could do with some extra eyeballs - especially the error codes.
>
> I'm not sure if we need to adjust the test in pg_dump.c - maybe we
> should look at using the same test as the backend (typlen == -1) to
> disambiguate real arrays from subscriptable base types.
>

The attached version removes all the places we had NAMEDATALEN - 2
restrictions on object names. If there's no objection I will commit this
shortly, as I think it now does all the previously agreed things.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> The attached version removes all the places we had NAMEDATALEN - 2
> restrictions on object names. If there's no objection I will commit this
> shortly, as I think it now does all the previously agreed things.

It needs some work yet, but I'll go over it and commit it.

BTW, just idly looking at this, I'm wondering whether we couldn't
now support arrays of domains too. If record_in/record_out work for
array elements, why wouldn't domain_in/domain_out?

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> The attached version removes all the places we had NAMEDATALEN - 2
>> restrictions on object names. If there's no objection I will commit this
>> shortly, as I think it now does all the previously agreed things.
>>
>
> It needs some work yet, but I'll go over it and commit it.
>
> BTW, just idly looking at this, I'm wondering whether we couldn't
> now support arrays of domains too. If record_in/record_out work for
> array elements, why wouldn't domain_in/domain_out?
>
>
>

Good question. I'm all in favor of doing that. Presumably we would not
in the case of a domain of an array type?

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>>> Attached is my rework of David Fetter's array of composites patch. It
>>> has all the agreed modifications and checks, except altering the name
>>> mangling.

Applied with revisions. There are some loose ends yet:

* I didn't do anything about arrays of domains. Although I think they'd
basically work, there's one nasty fly in the ointment, which is ALTER
DOMAIN ADD CONSTRAINT. get_rels_with_domain() is not smart enough to
detect arrays of domains, and its callers are not nearly smart enough to
apply their checks to arrays. So I think this had better wait for 8.4.

BTW, I realized there's an existing bug here as of 8.2: when I enabled
domains over domains I didn't do anything with get_rels_with_domain().
Fortunately this is a relatively easy thing to deal with, we can just
recurse to find columns of derived domain types, which the callers don't
really need to treat any differently than they do now. I'll go fix
that part.

* The feature leaves something to be desired in terms of usability,
because array[row()] doesn't work:

regression=# create type foo as (f1 int, f2 int);
CREATE TYPE
regression=# create table bar (ff1 foo[]);
CREATE TABLE
regression=# insert into bar values(array[row(1,2),row(3,4)]);
ERROR: could not find array type for data type record
regression=#

You can only get it to work if you plaster ::foo onto *each* row()
construct. Ugh. This didn't seem trivial to improve.

* I'm a bit concerned about dump order. If a user wants to create
types named "foo" and "_foo", he can, but it will only work if he
makes "_foo" first --- else the derived type for foo is in the way.
Since pg_dump has no clue about that constraint, it might easily
dump "foo" first leading to an unrestorable dump. The most usable
solution would be to auto-rename previously created array types,
but I dunno how implementable that would be.

regards, tom lane

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> * I'm a bit concerned about dump order. If a user wants to create
> types named "foo" and "_foo", he can, but it will only work if he
> makes "_foo" first --- else the derived type for foo is in the way.
> Since pg_dump has no clue about that constraint, it might easily
> dump "foo" first leading to an unrestorable dump. The most usable
> solution would be to auto-rename previously created array types,
> but I dunno how implementable that would be.

BTW, why exactly do we need array types to have names at all? The only
user-visible way to refer to these types is always by foo[] isn't it?

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

Gregory Stark wrote:
> "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>
>
>> * I'm a bit concerned about dump order. If a user wants to create
>> types named "foo" and "_foo", he can, but it will only work if he
>> makes "_foo" first --- else the derived type for foo is in the way.
>> Since pg_dump has no clue about that constraint, it might easily
>> dump "foo" first leading to an unrestorable dump. The most usable
>> solution would be to auto-rename previously created array types,
>> but I dunno how implementable that would be.
>>
>
> BTW, why exactly do we need array types to have names at all? The only
> user-visible way to refer to these types is always by foo[] isn't it?
>
>

I think you can use the _foo name, but it would certainly be an odd
thing to do.

I'd be happy to get rid of the name, or at least make it something very
unlikely indeed, but Tom didn't want to move too far from our present
naming convention. I am now wondering if we shouldn't at lest append
_arr or some such to the array type name, similarly to what we do for
generated sequence and index names.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Gregory Stark wrote:
>> BTW, why exactly do we need array types to have names at all?

Because typname is part of the primary key for pg_type ...

>> The only
>> user-visible way to refer to these types is always by foo[] isn't it?

> I think you can use the _foo name, but it would certainly be an odd
> thing to do.

There is *tons* of legacy code that uses _foo, mainly because there was
a time when we didn't support the [] notation in a lot of places where
types can be named. There still are some places, in fact:

regression=# alter type widget[] set schema public;
ERROR: syntax error at or near "["
LINE 1: alter type widget[] set schema public;
^
regression=# alter type _widget set schema public;
ERROR: cannot alter array type widget[]
HINT: You can alter type widget, which will alter the array type as well.
regression=#

That particular one may not need fixed (anymore) but the real problem is
the torches-and-pitchforks session that will ensue if we break legacy
code for no reason beyond cosmetics.

IIRC some of the contrib modules still have instances of _foo in
their SQL scripts.

regards, tom lane

Tom Lane wrote:
>
> There is *tons* of legacy code that uses _foo, mainly because there was
> a time when we didn't support the [] notation in a lot of places where
> types can be named. There still are some places, in fact:
>
> regression=# alter type widget[] set schema public;
> ERROR: syntax error at or near "["
> LINE 1: alter type widget[] set schema public;
> ^
> regression=# alter type _widget set schema public;
> ERROR: cannot alter array type widget[]
> HINT: You can alter type widget, which will alter the array type as well.
> regression=#
>
> That particular one may not need fixed (anymore) but the real problem is
> the torches-and-pitchforks session that will ensue if we break legacy
> code for no reason beyond cosmetics.
>
> IIRC some of the contrib modules still have instances of _foo in
> their SQL scripts.
>

Then I think we need to work out a way to make pg_dump smart enough to
dump things in the right order.

Can we perhaps explicitly deprecate using the type name to refer to
array types?

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Then I think we need to work out a way to make pg_dump smart enough to
> dump things in the right order.

That's not really the most preferable solution, I think, seeing that it
still leaves the user with the problem of having to create the types in
the right order to start with.

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> Then I think we need to work out a way to make pg_dump smart enough to
>> dump things in the right order.
>>
>
> That's not really the most preferable solution, I think, seeing that it
> still leaves the user with the problem of having to create the types in
> the right order to start with.
>
>
>

I'm not sure we can keep the _foo convention and avoid that. If we can't
find something sensible pretty quickly, I'd vote to revert the new name
mangling piece (but keep the typarray mapping column), deprecate the use
of the _foo convention, and abandon it next release. It would be a pity
though.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Tom Lane wrote:
>> That's not really the most preferable solution, I think, seeing that it
>> still leaves the user with the problem of having to create the types in
>> the right order to start with.

> I'm not sure we can keep the _foo convention and avoid that.

Auto-rename. I'm working on a patch now, and it doesn't look like it'll
be too awful. Will post it for comments when it's working.

> ... I'd vote to revert the new name
> mangling piece (but keep the typarray mapping column), deprecate the use
> of the _foo convention, and abandon it next release.

I came across a comment in the source that says PG has been using _foo
for arrays since 3.1 (!). I don't think we can get away with changing
it, certainly not with only one release cycle's notice.

The current code is OK from a compatibility point of view, since it only
changes _foo to something else in situations where the old way would've
failed outright. I think we need to preserve that property ...

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> Tom Lane wrote:
>>
>>> That's not really the most preferable solution, I think, seeing that it
>>> still leaves the user with the problem of having to create the types in
>>> the right order to start with.
>>>
>
>
>> I'm not sure we can keep the _foo convention and avoid that.
>>
>
> Auto-rename. I'm working on a patch now, and it doesn't look like it'll
> be too awful. Will post it for comments when it's working.
>

Ok, cool. I look forward to it.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Tom Lane wrote:
>> Auto-rename. I'm working on a patch now, and it doesn't look like it'll
>> be too awful. Will post it for comments when it's working.

> Ok, cool. I look forward to it.

Here's a bare-bones patch (no doc or regression tests). Seems to work.
Anyone think this is too ugly a way to proceed?

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> Tom Lane wrote:
>>
>>> Auto-rename. I'm working on a patch now, and it doesn't look like it'll
>>> be too awful. Will post it for comments when it's working.
>>>
>
>
>> Ok, cool. I look forward to it.
>>
>
> Here's a bare-bones patch (no doc or regression tests). Seems to work.
> Anyone think this is too ugly a way to proceed?
>
>
>

Summarising the behaviour as I understand it:

. if you never name a type/table with a name beginning with underscore,
behaviour is as expected - type foo gets array type _foo
. if you create a type foo and then create a type _foo, the array type
for foo will first be renamed to __foo, and the new array type for _foo
will be ___foo
. if you create type _foo and then create type foo, the corresponding
array types will be __foo and ___foo as per my patch, with no renaming
required.

I think I like it. Certainly seems to get round the ordering problem nicely.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Summarising the behaviour as I understand it:

> . if you never name a type/table with a name beginning with underscore,
> behaviour is as expected - type foo gets array type _foo
> . if you create a type foo and then create a type _foo, the array type
> for foo will first be renamed to __foo, and the new array type for _foo
> will be ___foo
> . if you create type _foo and then create type foo, the corresponding
> array types will be __foo and ___foo as per my patch, with no renaming
> required.

> I think I like it. Certainly seems to get round the ordering problem nicely.

At least as far as the user's names are concerned. There's some
ordering dependency for the names that the array types end up with,
but we had that problem already; and AFAIK it shouldn't create any
big issue for dump/restore.

BTW, I forgot to mention that this patch also fixes an oversight in the
original patch: we all missed the fact that ALTER TABLE RENAME didn't
rename the rowtype's array type.

regards, tom lane

Tom Lane wrote:
>
>> I think I like it. Certainly seems to get round the ordering problem nicely.
>>
>
> At least as far as the user's names are concerned. There's some
> ordering dependency for the names that the array types end up with,
> but we had that problem already; and AFAIK it shouldn't create any
> big issue for dump/restore.
>

There will only be an issue if you use table/type names beginning with
underscore, right? And I don't think it will matter because nobody has
been relying on that to date as we haven't had array types for those. We
should probably document that relying on the array name is both fragile
and unnecessary.

> BTW, I forgot to mention that this patch also fixes an oversight in the
> original patch: we all missed the fact that ALTER TABLE RENAME didn't
> rename the rowtype's array type.
>
>

Oh, good catch. Sorry about that.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> There will only be an issue if you use table/type names beginning with
> underscore, right? And I don't think it will matter because nobody has
> been relying on that to date as we haven't had array types for those. We
> should probably document that relying on the array name is both fragile
> and unnecessary.

I added this to the CREATE TYPE reference page, which AFAIR is the only
place that mentions the naming convention at all:

<para>
Before <productname>PostgreSQL</productname> version 8.3, the name of
a generated array type was always exactly the element type's name with one
underscore character (<literal>_</literal>) prepended. (Type names were
therefore restricted in length to one less character than other names.)
While this is still usually the case, the array type name may vary from
this in case of maximum-length names or collisions with user type names
that begin with underscore. Writing code that depends on this convention
is therefore deprecated. Instead, use
<structname>pg_type</>.<structfield>typarray</> to locate the array type
associated with a given type.
</para>

<para>
It may be advisable to avoid using type and table names that begin with
underscore. While the server will change generated array type names to
avoid collisions with user-given names, there is still risk of confusion,
particularly with old client software that may assume that type names
beginning with underscores always represent arrays.
</para>

regards, tom lane

TODO marked as done:

o -Add support for arrays of complex types

I assume this is _not_ done, as stated below:

o Add support for arrays of domains

I will add a URL for this item:

http://archives.postgresql.org/pgsql-patches/2007-05/msg00114.php

---------------------------------------------------------------------------

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> >>> Attached is my rework of David Fetter's array of composites patch. It
> >>> has all the agreed modifications and checks, except altering the name
> >>> mangling.
>
> Applied with revisions. There are some loose ends yet:
>
> * I didn't do anything about arrays of domains. Although I think they'd
> basically work, there's one nasty fly in the ointment, which is ALTER
> DOMAIN ADD CONSTRAINT. get_rels_with_domain() is not smart enough to
> detect arrays of domains, and its callers are not nearly smart enough to
> apply their checks to arrays. So I think this had better wait for 8.4.
>
> BTW, I realized there's an existing bug here as of 8.2: when I enabled
> domains over domains I didn't do anything with get_rels_with_domain().
> Fortunately this is a relatively easy thing to deal with, we can just
> recurse to find columns of derived domain types, which the callers don't
> really need to treat any differently than they do now. I'll go fix
> that part.
>
> * The feature leaves something to be desired in terms of usability,
> because array[row()] doesn't work:
>
> regression=# create type foo as (f1 int, f2 int);
> CREATE TYPE
> regression=# create table bar (ff1 foo[]);
> CREATE TABLE
> regression=# insert into bar values(array[row(1,2),row(3,4)]);
> ERROR: could not find array type for data type record
> regression=#
>
> You can only get it to work if you plaster ::foo onto *each* row()
> construct. Ugh. This didn't seem trivial to improve.
>
> * I'm a bit concerned about dump order. If a user wants to create
> types named "foo" and "_foo", he can, but it will only work if he
> makes "_foo" first --- else the derived type for foo is in the way.
> Since pg_dump has no clue about that constraint, it might easily
> dump "foo" first leading to an unrestorable dump. The most usable
> solution would be to auto-rename previously created array types,
> but I dunno how implementable that would be.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: Don't 'kill -9' the postmaster

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

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