Re: [PATCHES] updated WIP: arrays of composites

2007-05-13 Thread Bruce Momjian

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 [EMAIL PROTECTED] 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  [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 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] updated WIP: arrays of composites

2007-05-11 Thread Tom Lane
Andrew Dunstan [EMAIL PROTECTED] 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


Re: [PATCHES] updated WIP: arrays of composites

2007-05-11 Thread Gregory Stark

Tom Lane [EMAIL PROTECTED] 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


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


Re: [PATCHES] updated WIP: arrays of composites

2007-05-11 Thread Andrew Dunstan



Tom Lane wrote:

Andrew Dunstan [EMAIL PROTECTED] 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

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

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


Re: [PATCHES] updated WIP: arrays of composites

2007-05-11 Thread Tom Lane
Andrew Dunstan [EMAIL PROTECTED] 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

---(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] updated WIP: arrays of composites

2007-05-11 Thread Andrew Dunstan


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


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

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


Re: [PATCHES] updated WIP: arrays of composites

2007-05-11 Thread Andrew Dunstan



Tom Lane wrote:

Andrew Dunstan [EMAIL PROTECTED] 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

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

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


Re: [PATCHES] updated WIP: arrays of composites

2007-05-11 Thread Tom Lane
Andrew Dunstan [EMAIL PROTECTED] 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

Index: src/backend/catalog/heap.c
===
RCS file: /cvsroot/pgsql/src/backend/catalog/heap.c,v
retrieving revision 1.319
diff -c -r1.319 heap.c
*** src/backend/catalog/heap.c  11 May 2007 17:57:11 -  1.319
--- src/backend/catalog/heap.c  11 May 2007 23:18:51 -
***
*** 797,802 
--- 797,803 
  {
Relationpg_class_desc;
Relationnew_rel_desc;
+   Oid old_type_oid;
Oid new_type_oid;
Oid new_array_oid = InvalidOid;
  
***
*** 815,820 
--- 816,842 
 errmsg(relation \%s\ already exists, 
relname)));
  
/*
+* Since we are going to create a rowtype as well, also check for
+* collision with an existing type name.  If there is one and it's
+* an autogenerated array, we can rename it out of the way; otherwise
+* we can at least give a good error message.
+*/
+   old_type_oid = GetSysCacheOid(TYPENAMENSP,
+ 
CStringGetDatum(relname),
+ 
ObjectIdGetDatum(relnamespace),
+ 0, 0);
+   if (OidIsValid(old_type_oid))
+   {
+   if (!moveArrayTypeName(old_type_oid, relname, relnamespace))
+   ereport(ERROR,
+   (errcode(ERRCODE_DUPLICATE_OBJECT),
+errmsg(type \%s\ already exists, 
relname),
+errhint(A relation has an associated 
type of the same name, 
+so you must use a 
name that doesn't conflict 
+with any existing 
type.)));
+   }
+ 
+   /*
 * Allocate an OID for the relation, unless we were told what to use.
 *
 * The OID will be the relfilenode as well, so make sure it doesn't
***
*** 861,868 
 * Since defining a relation also defines a complex type, we add a new
 * system type corresponding to the new relation.
 *
!* NOTE: we could get a unique-index failure here, in case the same name
!* has already been used for a type.
 */
new_type_oid = AddNewRelationType(relname,
  
relnamespace,
--- 883,891 
 * Since defining a relation also defines a complex type, we add a new
 * system type corresponding to the new relation.
 *
!* NOTE: we could get a unique-index failure here, in case someone else 
is
!* creating the same type name in parallel but hadn't committed yet
!* when we checked for a duplicate name above.
 */
new_type_oid = AddNewRelationType(relname,
  
relnamespace,
Index: src/backend/catalog/pg_type.c
===
RCS file: /cvsroot/pgsql/src/backend/catalog/pg_type.c,v
retrieving revision 1.112
diff -c -r1.112 pg_type.c
*** src/backend/catalog/pg_type.c   11 May 2007 17:57:12 -  1.112
--- src/backend/catalog/pg_type.c   11 May 2007 23:18:51 -
***
*** 15,20 
--- 15,21 
  #include postgres.h
  
  #include access/heapam.h
+ #include access/xact.h
  #include catalog/dependency.h
  #include catalog/indexing.h
  #include catalog/pg_namespace.h
***
*** 26,31 
--- 27,33 
  #include utils/acl.h
  #include utils/builtins.h
  #include utils/fmgroids.h
+ #include utils/lsyscache.h
  #include utils/syscache.h
  
  
***
*** 551,580 
  
  /*
   * TypeRename
!  *This renames a type
   *
!  * Note: any associated array type is *not* renamed; caller must make
!  * another call to handle that case.  Currently this is only used for
!  * renaming types associated with tables, for which there are no arrays.
   */
  void
! TypeRename(const char *oldTypeName, Oid typeNamespace,
!  const char *newTypeName)
  {
Relationpg_type_desc;
HeapTuple   tuple;
  
pg_type_desc = heap_open(TypeRelationId, RowExclusiveLock);
  
!   tuple = SearchSysCacheCopy(TYPENAMENSP,
!

Re: [PATCHES] updated WIP: arrays of composites

2007-05-11 Thread Andrew Dunstan



Gregory Stark wrote:

Tom Lane [EMAIL PROTECTED] 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

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


Re: [PATCHES] updated WIP: arrays of composites

2007-05-11 Thread Andrew Dunstan



Tom Lane wrote:

Andrew Dunstan [EMAIL PROTECTED] 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


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


Re: [PATCHES] updated WIP: arrays of composites

2007-05-11 Thread Tom Lane
Andrew Dunstan [EMAIL PROTECTED] 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

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

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


Re: [PATCHES] updated WIP: arrays of composites

2007-05-11 Thread Andrew Dunstan



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

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

  http://archives.postgresql.org


Re: [PATCHES] updated WIP: arrays of composites

2007-05-11 Thread Tom Lane
Andrew Dunstan [EMAIL PROTECTED] 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

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


Re: [PATCHES] updated WIP: arrays of composites

2007-05-11 Thread Tom Lane
Andrew Dunstan [EMAIL PROTECTED] 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 productnamePostgreSQL/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
   structnamepg_type/.structfieldtyparray/ 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

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


Re: [PATCHES] updated WIP: arrays of composites

2007-05-10 Thread Tom Lane
Andrew Dunstan [EMAIL PROTECTED] 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

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


Re: [PATCHES] updated WIP: arrays of composites

2007-05-10 Thread Andrew Dunstan



Tom Lane wrote:

Andrew Dunstan [EMAIL PROTECTED] 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

---(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] updated WIP: arrays of composites

2007-05-07 Thread Tom Lane
Andrew Dunstan [EMAIL PROTECTED] 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

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