Re: Review: Typed Table

On tis, 2010-01-19 at 01:01 +0900, Hitoshi Harada wrote:
> I reviewed this patch today.

Thank you for this very thorough and helpful review. Comments below and
a new patch attached.

> * in namespace.c
> I didn't see why this file has been changed.
> case 0:
> ereport(ERROR,
> (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("improper qualified name (zero-length name list)")));
> break;
> in DeconstructQualifiedName().

Yeah, that doesn't really belong here. I ran into this problem early in
the hacking that an accidental zero-length list was reported as "too
many dotted names". But the final patch doesn't use any zero-length
dotted lists anymore. :-) The error message could still be clarified,
but this can be addressed separately if desired. I took it out for now.

> * Crash with wrong column name
> regression=# create type persons_type as (name text, bdate date);
> CREATE TYPE
> regression=# create table persons of persons_type (myname with options
> not null);
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Succeeded.

Fixed.

> * Conflict between transactions
> I'm not sure if this is related with the patch but I met this situation;
>
> A: regression=# create type persons_type as (name text, bdate date);
> A: CREATE TYPE
>
> A: regression=# begin;
> A: BEGIN
>
> A: regression=# drop type persons_type;
> A: DROP TYPE
>
> B: regression=# create table persons of persons_type; (LOCK)
> A: regression=# rollback;
> A: ROLLBACK
> B: CREATE TABLE
>
> B: regression=# drop table persons;
> B: DROP TABLE
>
> A: regression=# begin;
> A: BEGIN
>
> A: regression=# drop type persons_type;
> A: DROP TYPE
>
> B: regression=# create table persons of persons_type; (NO LOCK)
> B: CREATE TABLE
>
> A: regression=# commit;
> A: COMMIT
>
> B: regression=# select 'persons_type'::regtype;
> B: ERROR: type "persons_type" does not exist
> B: LINE 1: select 'persons_type'::regtype;
>
> I have at all no idea why the second create table doesn't lock.

Well, if you try the same thing with CREATE FUNCTION foo() RETURNS
persons_type AS $$ blah $$ LANGUAGE plpythonu; or some similar cases,
there is also no lock. You will notice that (some/many?) DDL statements
actually behave very poorly against concurrent other DDL. Against that
background, however, the real question is why the first case *does*
lock. I don't know.

> * Comment needed in pg_dump.h
> Please add comment on reloftype of struct _tableInfo

Fixed.

> * Consistency between add/drop and rename
> Typed table can rename its column but can NOT add/drop column. Is this
> what the spec requires? IMHO, if it allows rename, do so for add/drop
> and if do not allow add/drop, do not so rename.

I added a prohibition against renaming.

I have a follow-up patch that I haven't been able to finish that adds
ALTER TYPE stuff to do add/dropping/renaming on the type. I will submit
it once we get this patch finalized and I find some time.