Problems with ALTER DOMAIN patch

I've been looking at the recently-committed ALTER DOMAIN patch, and I
think it's got some serious if not fatal problems.  Specifically, the
approach to adding/dropping constraints associated with domains doesn't
work.

1. Insufficient locking, guise 1: there's no protection against someone
else dropping a column or whole table between the time you find a
pg_attribute entry in get_rels_with_domain and the time you actually
process it in AlterDomainNotNull or AlterDomainAddConstraint.  The code
appears to think that holding RowExclusiveLock on pg_attribute will
protect it somehow, but that doesn't (and shouldn't) do any such thing.
This will result in at best an elog and at worst coredump when you try
to scan the no-longer-present table or column.

2. Insufficient locking, guise 2: there's no protection against someone
else adding a column or table while you're processing an ALTER DOMAIN,
either.  This means that constraint checks will be missed.  Example:

<< backend 1 >>

regression=# create domain mydom int4;
CREATE DOMAIN
regression=# begin;
BEGIN
regression=# alter domain mydom set not null;
ALTER DOMAIN

<< don't commit yet; in backend 2 do >>

regression=# create table foo (f1 mydom);
CREATE TABLE
regression=# insert into foo values(null);
INSERT 149688 1

<< now in backend 1: >>

regression=# commit;
COMMIT

<< now in backend 2: >>

regression=# insert into foo values(null);
ERROR:  Domain mydom does not allow NULL values
regression=# select * from foo;f1
----

(1 row)

Not a very watertight domain constraint, is it?  The begin/commit is not
necessary to cause a failure, it just makes it easy to make the window
for failure wide enough to hit in a manually entered example.

3. Too much locking, guise 1: the ALTER DOMAIN command will acquire
exclusive lock on every table that it scans, and will hold all these
locks until it commits.  This can easily result in deadlocks --- against
other ALTER DOMAIN commands, or just against any random other
transaction that is unlucky enough to try to write any two tables
touched by the ALTER DOMAIN.  AFAICS you don't need an exclusive lock,
you just want to prevent updates of the table until the domain changes
are committed, so ShareLock would be sufficient; that would reduce but
not eliminate the risk of deadlock.

4. Too much locking, guise 2: the ExclusiveLock acquired on pg_class by
get_rels_with_domain has no useful effect, since it's released again
at the end of the scan; it does manage to shut down most sorts of schema
changes while get_rels_with_domain runs, however.  This is bad enough,
but:

5. Performance sucks.  In the regression database on my machine, "alter
domain mydom set not null" takes over six seconds --- that's for a
freshly created domain that's not used *anywhere*.  This can be blamed
entirely on the inefficient implementation of get_rels_with_domain.
In a database with more tables performance would get much worse; it's
basically O(N^2).  And it's holding ExclusiveLock on pg_class the whole
time :-(.  (A reasonably efficient way to make the same search would be
to use pg_depend to look for columns that depend on the domain type ---
this might find a few indirect dependencies, but it would certainly be
lots faster than repeated seqscans over pg_attribute.)

6. Permission bogosity: as per discussion yesterday, ownership of a
schema does not grant ownership rights on contained objects.

7. No mechanism for causing constraint changes to actually propagate
after they are made.  This is more a fault of the design of the domain
constraint patch than it is of the alter patch, but nonetheless alter is
what exposes it.  The problem is particularly acute because you chose to
insert a domain's constraint expressions into coercion operations at
expression parsing time, which is far too early.  A stored rule that has
a coerce-to-domain operation in it will have a frozen idea of what
constraints it should be enforcing.  Probably the expression tree should
just have a "CoerceToDomain foo" node in it, and at executor startup
this node would have to look to the pg_type entry for foo to see exactly
what it should be enforcing at the moment.


Some of these are fixable, but I don't actually see any fix for point 2
short of creating some entirely new locking convention.  Currently, only
relations can be locked, but you'd really need an enforceable lock on
the type itself to make a watertight solution, I think.  Since we've
never had any sort of supported ALTER TYPE operation before, the issue
hasn't come up before ...
        regards, tom lane