Re: Review: Typed Table

I reviewed this patch today.

Overview:
Almost everything is OK. Applied with few hunks (in psql/describle.c 2
lines offset), compiled without warnings, passed regression tests. The
results of advertised queries are as expected. Coding style is of
course satisfied. Since this is utility changes performance does not
go down.

* 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().

* 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.

* 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.

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

* 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 read SQL standard about typed table for few minutes but did not find
any problems. It mentions about "reference column" as the patch
document says and I, too, think our oid mechanism will do for that as
the doc says.

Regards,

--
Hitoshi Harada

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.

2010/1/26 Peter Eisentraut <peter_e(at)gmx(dot)net>:
> 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.
>
OK, I confirmed all the issues relevant to the patch were fixed. I'm
not so familiar with transaction detail, so I leave it as a known
issue.

I found ereport() in MergeAttributes() should be indented but except
for that there's no issue. So I think I've done my review.

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

I'll look at it when it's ready.

Regards,

--
Hitoshi Harada

Everyone,

We could use some help. Anyone's got an idea what could be causing the
behavior described below?

On mån, 2010-01-25 at 21:45 +0200, Peter Eisentraut wrote:
> On tis, 2010-01-19 at 01:01 +0900, Hitoshi Harada wrote:
> > * 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.

Peter Eisentraut escribió:
> Everyone,
>
> We could use some help. Anyone's got an idea what could be causing the
> behavior described below?

I wonder if the problem is that you're missing a recheck on the type's
existence after you've grabbed the lock on it, similar to what
shdepLockAndCheckObject does. Maybe the second attempt to create the
table doesn't block because pg_depends contents are different? It seems
very strange.

> On mån, 2010-01-25 at 21:45 +0200, Peter Eisentraut wrote:
> > On tis, 2010-01-19 at 01:01 +0900, Hitoshi Harada wrote:
> > > * 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.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Peter Eisentraut wrote:
> Everyone,
>
> We could use some help. Anyone's got an idea what could be causing the
> behavior described below?
>
>
> On mån, 2010-01-25 at 21:45 +0200, Peter Eisentraut wrote:
>> On tis, 2010-01-19 at 01:01 +0900, Hitoshi Harada wrote:
>>> * 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.

Types are cached in typcache. At the first CREATE TABLE, the type is not
in cache, and lookup_type_cache() (by the call to
lookup_rowtype_tupdesc() in transformOfType()) calls relation_open()
which blocks. On the second call, however, it's already in the cache,
and relation_open is not called.

ISTM you should explicitly grab a lock on the of-type at some point, to
make sure it doesn't get dropped while you're busy creating the table.
How do we protect against that for the types used in columns? For
example, if you do "CREATE TABLE (foo mytype)", and someone tries to
"drop mytype" simultaneously?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> ISTM you should explicitly grab a lock on the of-type at some point, to
> make sure it doesn't get dropped while you're busy creating the table.
> How do we protect against that for the types used in columns?

We don't. There is no concept of a lock on a type.

For scalar types this is more or less irrelevant anyway, since a scalar
has no substructure that can be altered in any interesting way. I'm not
sure how hard we ought to work on making composites behave differently.
I think it's as likely to cause problems as solve them.

regards, tom lane

On tor, 2010-01-28 at 10:34 -0500, Tom Lane wrote:
> Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> > ISTM you should explicitly grab a lock on the of-type at some point, to
> > make sure it doesn't get dropped while you're busy creating the table.
> > How do we protect against that for the types used in columns?
>
> We don't. There is no concept of a lock on a type.
>
> For scalar types this is more or less irrelevant anyway, since a scalar
> has no substructure that can be altered in any interesting way. I'm not
> sure how hard we ought to work on making composites behave differently.
> I think it's as likely to cause problems as solve them.

The right thing would probably be SELECT FOR SHARE on the pg_type row,
but I don't see that sort of thing used anywhere else in system catalog
changes.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> The right thing would probably be SELECT FOR SHARE on the pg_type row,
> but I don't see that sort of thing used anywhere else in system catalog
> changes.

If we were to do it the right thing would just be to define a locktag
for type OIDs and add appropriate locking calls all over the system.
But that would be a large, invasive change that seems far outside the
scope of this patch, and certainly much beyond what can get done for
9.0.

(Actually, if memory serves there is some notion of locking on arbitrary
catalog objects already in the DROP code, so there probably is a
suitable locktag defined already. But getting ALTER and generic type
references to play along is still a major project, and I'm not convinced
about the cost/benefit ...)

regards, tom lane

On tor, 2010-01-28 at 00:43 +0900, Hitoshi Harada wrote:
> OK, I confirmed all the issues relevant to the patch were fixed. I'm
> not so familiar with transaction detail, so I leave it as a known
> issue.

I have applied this now, because it appeared that the locking issue is a
known more general problem.