Arrays of Complex Types

David Fetter <david@fetter.org> writes:

1. In src/backend/commands/tablecmds.c, change DefineRelation as
follows:
* After the first call to heap_create_with_catalog, construct and
do another call to for the array type.

I'm still not happy about the idea of doing this for every relation
(and doing it for sequences and indexes would be the height of
wastefulness). How about we only do it for composite types?

* Add an appropriate pg_depend entry.
2. Change RemoveRelation to reflect the above.

You only need one of those two: either you drop by hand or you let the
dependency machinery deal with it. Not both.

Does the above make sense? Have I missed anything critical?

Ummm ... making it actually work? Possibly that just falls out, but I'm
not sure.

If it turns out that it does Just Work, you might take a stab at arrays
of domains too.

regards, tom lane

Tom Lane wrote:

David Fetter <david@fetter.org> writes:

1. In src/backend/commands/tablecmds.c, change DefineRelation as
follows:
* After the first call to heap_create_with_catalog, construct and
do another call to for the array type.

I'm still not happy about the idea of doing this for every relation
(and doing it for sequences and indexes would be the height of
wastefulness). How about we only do it for composite types?

I'm not happy about that. I agree that indexes and sequences should not be
done, but can we please do plain table types? I would be OK if we skipped
catalog tables, if that would make you happier.

cheers

andrew

On Fri, Mar 02, 2007 at 06:42:14PM -0600, Andrew Dunstan wrote:

I'm still not happy about the idea of doing this for every relation
(and doing it for sequences and indexes would be the height of
wastefulness). How about we only do it for composite types?

I'm not happy about that. I agree that indexes and sequences should not be
done, but can we please do plain table types? I would be OK if we skipped
catalog tables, if that would make you happier.

Two thoughts:

1. Make the array types only when someone actually uses them (create a
table with it or something).

2. Make a command: CREATE TYPE ARRAY OF "foo";

The latter has the benefit of not restricting it to an arbitrary choice
of types, you could accept both domains and composite types here. I
don't think it's unreasonable to require people to predeclare their
usage of array-of-compostite-type.

Perhaps change the word "CREATE" to "DECLARE". I'm thinking of the
explicit declaration of shell types as precedent here.

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Martijn van Oosterhout wrote:

On Fri, Mar 02, 2007 at 06:42:14PM -0600, Andrew Dunstan wrote:

I'm still not happy about the idea of doing this for every relation
(and doing it for sequences and indexes would be the height of
wastefulness). How about we only do it for composite types?

I'm not happy about that. I agree that indexes and sequences should not be
done, but can we please do plain table types? I would be OK if we skipped
catalog tables, if that would make you happier.

Two thoughts:

1. Make the array types only when someone actually uses them (create a
table with it or something).

2. Make a command: CREATE TYPE ARRAY OF "foo";

The latter has the benefit of not restricting it to an arbitrary choice
of types, you could accept both domains and composite types here. I
don't think it's unreasonable to require people to predeclare their
usage of array-of-compostite-type.

Perhaps change the word "CREATE" to "DECLARE". I'm thinking of the
explicit declaration of shell types as precedent here.

#2 would be fine with me - not keen on #1.

cheers

andrew

On Sat, Mar 03, 2007 at 09:06:11AM -0500, Andrew Dunstan wrote:

Martijn van Oosterhout wrote:

On Fri, Mar 02, 2007 at 06:42:14PM -0600, Andrew Dunstan wrote:

I'm still not happy about the idea of doing this for every relation
(and doing it for sequences and indexes would be the height of
wastefulness). How about we only do it for composite types?

I'm not happy about that. I agree that indexes and sequences should not be
done, but can we please do plain table types? I would be OK if we skipped
catalog tables, if that would make you happier.

Two thoughts:

1. Make the array types only when someone actually uses them (create a
table with it or something).

This doesn't sound so great.

2. Make a command: CREATE TYPE ARRAY OF "foo";

The latter has the benefit of not restricting it to an arbitrary choice
of types, you could accept both domains and composite types here.

I'm thinking that DOMAINs over simple types should just automatically
get corresponding array types. We don't yet support DOMAINs over
complex types, but when we do, whatever we do with arrays of regular
complex types should apply the same way.

I don't think it's unreasonable to require people to predeclare
their usage of array-of-compostite-type.

Perhaps change the word "CREATE" to "DECLARE". I'm thinking of the
explicit declaration of shell types as precedent here.

#2 would be fine with me - not keen on #1.

Per your earlier suggestion in IRC, I'm picturing a polymorphic
function like

pg_catalog.array_for(typepoid OID)
pg_catalog.array_for(typename NAME)
pg_catalog.array_for(typenamespace NAME, typename NAME)

I don't see a good reason to allow putting array types in a different
schema from their base types.

Cheers,
D
--
David Fetter <david@fetter.org> http://fetter.org/
phone: +1 415 235 3778 AIM: dfetter666
Skype: davidfetter

Remember to vote!
Consider donating to PostgreSQL: http://www.postgresql.org/about/donate

"David Fetter" <david@fetter.org> writes:

2. Make a command: CREATE TYPE ARRAY OF "foo";

The latter has the benefit of not restricting it to an arbitrary choice
of types, you could accept both domains and composite types here.

I'm thinking that DOMAINs over simple types should just automatically
get corresponding array types. We don't yet support DOMAINs over
complex types, but when we do, whatever we do with arrays of regular
complex types should apply the same way.

Just a thought, but if we supported domains over complex types we could kill
two birds with one stone and say when you create a domain the array type is
created and that's how you have to refer to arrays over complex types.
Probably doesn't make anything easier though. Just a thought.

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

David Fetter wrote:

Per your earlier suggestion in IRC, I'm picturing a polymorphic
function like

pg_catalog.array_for(typepoid OID)
pg_catalog.array_for(typename NAME)
pg_catalog.array_for(typenamespace NAME, typename NAME)

I don't see a good reason to allow putting array types in a different
schema from their base types.

Actually, I think I prefer Martijns simple SQL extension suggestion better.

cheers

andrew

On Fri, Mar 02, 2007 at 06:59:50PM -0500, Tom Lane wrote:

David Fetter <david@fetter.org> writes:

1. In src/backend/commands/tablecmds.c, change DefineRelation as
follows:
* After the first call to heap_create_with_catalog, construct and
do another call to for the array type.

I'm still not happy about the idea of doing this for every relation
(and doing it for sequences and indexes would be the height of
wastefulness). How about we only do it for composite types?

How about doing it for user-defined tables, views and composite types,
and skipping ?

* Add an appropriate pg_depend entry.
2. Change RemoveRelation to reflect the above.

You only need one of those two: either you drop by hand or you let the
dependency machinery deal with it. Not both.

pg_depend it is, then :)

Does the above make sense? Have I missed anything critical?

Ummm ... making it actually work? Possibly that just falls out, but
I'm not sure.

If it turns out that it does Just Work, you might take a stab at
arrays of domains too.

OK.

I noticed something in src/backend/commands/tablecmds.c which worries
me, namely that it ignores functions and views. It should at least be
checking that the typeoid isn't in pg_proc.prorettype or
pg_proc.proargtypes, and if possible, the DECLARE section of pl/pgsql
functions.

Is there a way to do SQL at that place in the back-end, or is there
some different kind of Magick(TM) needed to access these kinds of
things at that level?

Cheers,
D
--
David Fetter <david@fetter.org> http://fetter.org/
phone: +1 415 235 3778 AIM: dfetter666
Skype: davidfetter

Remember to vote!
Consider donating to PostgreSQL: http://www.postgresql.org/about/donate

David Fetter <david@fetter.org> writes:

I noticed something in src/backend/commands/tablecmds.c which worries
me, namely that it ignores functions and views.

What?

regards, tom lane

On Tue, Mar 06, 2007 at 04:14:07PM -0500, Tom Lane wrote:

David Fetter <david@fetter.org> writes:

I noticed something in src/backend/commands/tablecmds.c which
worries me, namely that it ignores functions and views.

What?

The "it" in question is, find_composite_type_dependencies()

Cheers,
D
--
David Fetter <david@fetter.org> http://fetter.org/
phone: +1 415 235 3778 AIM: dfetter666
Skype: davidfetter

Remember to vote!
Consider donating to PostgreSQL: http://www.postgresql.org/about/donate

David Fetter <david@fetter.org> writes:

On Tue, Mar 06, 2007 at 04:14:07PM -0500, Tom Lane wrote:

David Fetter <david@fetter.org> writes:

I noticed something in src/backend/commands/tablecmds.c which
worries me, namely that it ignores functions and views.

What?

The "it" in question is, find_composite_type_dependencies()

All that that's interested in is whether there are stored values of the
datatype somewhere. Views don't have any storage, and a function
definition doesn't either.

regards, tom lane

On Tue, Mar 06, 2007 at 04:24:36PM -0500, Tom Lane wrote:

David Fetter <david@fetter.org> writes:

On Tue, Mar 06, 2007 at 04:14:07PM -0500, Tom Lane wrote:

David Fetter <david@fetter.org> writes:

I noticed something in src/backend/commands/tablecmds.c which
worries me, namely that it ignores functions and views.

What?

The "it" in question is, find_composite_type_dependencies()

All that that's interested in is whether there are stored values of the
datatype somewhere. Views don't have any storage, and a function
definition doesn't either.

I see. Perhaps I've misunderstood what this thing was for, then.
What is it that checks whether it's OK to change a composite type,
then?

Cheers,
D
--
David Fetter <david@fetter.org> http://fetter.org/
phone: +1 415 235 3778 AIM: dfetter666
Skype: davidfetter

Remember to vote!
Consider donating to PostgreSQL: http://www.postgresql.org/about/donate

I wrote:

I'd like to take the TODO item that reads, "Add support for arrays of
complex types," but before I start patching, I'd like to see whether
what I'm about to do makes any sense:

I've written up a patch intended to implement this on the
non-pg_catalog tables and VIEWs, but while it builds, it doesn't
initdb. Enclosed are the patch and the error log.

Any hints as to what I might look at?

Cheers,
David.

1. In src/backend/commands/tablecmds.c, change DefineRelation as
follows:

* After the first call to heap_create_with_catalog, construct and
do another call to for the array type.

* Add an appropriate pg_depend entry.

2. Change RemoveRelation to reflect the above.

3. Change TypeRename appropriately, whatever that turns out to be.

Does the above make sense? Have I missed anything critical?

Cheers,
D
--
David Fetter <david@fetter.org> http://fetter.org/
phone: +1 415 235 3778 AIM: dfetter666
Skype: davidfetter

Remember to vote!
Consider donating to PostgreSQL: http://www.postgresql.org/about/donate

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

--
David Fetter <david@fetter.org> http://fetter.org/
phone: +1 415 235 3778 AIM: dfetter666
Skype: davidfetter

Remember to vote!
Consider donating to PostgreSQL: http://www.postgresql.org/about/donate

David Fetter <david@fetter.org> writes:

I've written up a patch intended to implement this on the
non-pg_catalog tables and VIEWs, but while it builds, it doesn't
initdb. Enclosed are the patch and the error log.
Any hints as to what I might look at?

creating template1 database in /var/lib/pgsql/pgsql/src/test/regress/./tmp_check/data/base/1 ... ok
initializing pg_authid ... ok
initializing dependencies ... ok
creating system views ... ok
loading system objects' descriptions ... FATAL: cache lookup failed for type 11096
child process exited with exit code 1

That step of initdb creates a TEMP table ... maybe your patch doesn't
work for temp tables? Anyway, you're certainly far enough along there
that you could fire up the postmaster and reproduce the error in a
normal debugging environment.

regards, tom lane

On Sun, Mar 25, 2007 at 10:18:14PM -0400, Tom Lane wrote:

David Fetter <david@fetter.org> writes:

I've written up a patch intended to implement this on the
non-pg_catalog tables and VIEWs, but while it builds, it doesn't
initdb. Enclosed are the patch and the error log.
Any hints as to what I might look at?

creating template1 database in /var/lib/pgsql/pgsql/src/test/regress/./tmp_check/data/base/1 ... ok
initializing pg_authid ... ok
initializing dependencies ... ok
creating system views ... ok
loading system objects' descriptions ... FATAL: cache lookup failed for type 11096
child process exited with exit code 1

That step of initdb creates a TEMP table ... maybe your patch doesn't
work for temp tables? Anyway, you're certainly far enough along there
that you could fire up the postmaster and reproduce the error in a
normal debugging environment.

I've done that, and thanks to Andrew of Supernews, I've got a slightly
better patch, albeit one that bombs out at the same spot. In the
patch attached, it appears that TypeCreate is not doing the right
thing in pg_depend, either because I'm not invoking it right or
because it needs more machinery.

Any ideas?

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
phone: +1 415 235 3778 AIM: dfetter666
Skype: davidfetter

Remember to vote!
Consider donating to PostgreSQL: http://www.postgresql.org/about/donate

On Tue, Mar 27, 2007 at 11:08:44AM -0700, David Fetter wrote:

On Sun, Mar 25, 2007 at 10:18:14PM -0400, Tom Lane wrote:

David Fetter <david@fetter.org> writes:

I've written up a patch intended to implement this on the
non-pg_catalog tables and VIEWs, but while it builds, it doesn't
initdb. Enclosed are the patch and the error log.
Any hints as to what I might look at?

creating template1 database in /var/lib/pgsql/pgsql/src/test/regress/./tmp_check/data/base/1 ... ok
initializing pg_authid ... ok
initializing dependencies ... ok
creating system views ... ok
loading system objects' descriptions ... FATAL: cache lookup failed for type 11096
child process exited with exit code 1

That step of initdb creates a TEMP table ... maybe your patch
doesn't work for temp tables? Anyway, you're certainly far enough
along there that you could fire up the postmaster and reproduce
the error in a normal debugging environment.

I've done that, and thanks to Andrew of Supernews, I've got a
slightly better patch, albeit one that bombs out at the same spot.
In the patch attached, it appears that TypeCreate is not doing the
right thing in pg_depend, either because I'm not invoking it right
or because it needs more machinery.

Any ideas?

Pardon the self-follow-up.

Per further discussion with Andrew of Supernews and Merlin Moncure,
I've added a check for compound types and moved the creation of the
array type from DefineRelation in backend/commands/tablecmds.c to
heap_create_with_catalog in backend/catalog/heap.c.

It now initdb's successfully, but fails on a lot of regression tests.

Please find attached the new patch vs. CVS TIP and the regression test
output.

Am I on the right track here?

Cheers,
D
--
David Fetter <david@fetter.org> http://fetter.org/
phone: +1 415 235 3778 AIM: dfetter666
Skype: davidfetter

Remember to vote!
Consider donating to PostgreSQL: http://www.postgresql.org/about/donate

David Fetter wrote:

It now initdb's successfully, but fails on a lot of regression tests.

Please find attached the new patch vs. CVS TIP and the regression test
output.

The regression test output is useless without seeing the regression diffs.

cheers

andrew

On 2007-03-27, David Fetter <david@fetter.org> wrote:

Per further discussion with Andrew of Supernews and Merlin Moncure,
I've added a check for compound types and moved the creation of the
array type from DefineRelation in backend/commands/tablecmds.c to
heap_create_with_catalog in backend/catalog/heap.c.

You've still got the usage of the relation OID and the relation _type_ OID
reversed.

The array element type that you pass to TypeCreate must be the _type_ OID.

--
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services

On Wed, Mar 28, 2007 at 07:05:24AM -0000, Andrew - Supernews wrote:

On 2007-03-27, David Fetter <david@fetter.org> wrote:

Per further discussion with Andrew of Supernews and Merlin
Moncure, I've added a check for compound types and moved the
creation of the array type from DefineRelation in
backend/commands/tablecmds.c to heap_create_with_catalog in
backend/catalog/heap.c.

You've still got the usage of the relation OID and the relation
_type_ OID reversed.

The array element type that you pass to TypeCreate must be the
_type_ OID.

The attached patch takes it down to two regression test failures, also
attached:

The first is in type_sanity, which basically doesn't understand that
complex types now have array types associated with them and thinks
they're orphan array types, so it's actually the test that's not
right.

The second is in alter_table where ALTER TABLE ... SET SCHEMA doesn't
pick up the array types associated with the tables.

Andrew at Supernews also noticed that in general, the array type
doesn't change schemas when its base type does. Is this the intended
behavior? If not, should we change it globally?

Cheers,
D
--
David Fetter <david@fetter.org> http://fetter.org/
phone: +1 415 235 3778 AIM: dfetter666
Skype: davidfetter

Remember to vote!
Consider donating to PostgreSQL: http://www.postgresql.org/about/donate