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