Re: Problems with ALTER DOMAIN patch

On Tue, 2002-12-10 at 12:39, Tom Lane wrote:
> 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

Ok.. I obviously have to spend some time to figure out how locking works
and exactly what it affects.

I had incorrectly assumed that since dropping a column required removal
of the pg_attribute entry, that holding a RowExclusive on it would
prevent that.

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

Locking the entry in pg_type doesn't prevent that?  Afterall, something
does a test to see if the type exists prior to allowing the client to
add it.

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

I noticed a completed TODO item that allows multiple locks to be
obtained simultaneously, and had intended on using that for this -- but
was having a hard time tracking down an example.

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

Yeah... Trying to transfer the lock to the attributes -- which as you've
shown doesn't do what I thought.

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

Yes, I need to (and intend to) redo this with dependencies, but hadn't
figured out how.   I'm surprised it took 6 seconds though.  I hardly
notice any delay on a database with ~30 tables in it.

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

Patch submitted yesterday to correct this.

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

Thanks for the explanations.  I'll see if I can 1) fix my poor knowledge
of locking, 2) Add to my notes that I need to test stuff with Rules from
now on, and 3) correct the above items.
--
Rod Taylor <rbt@rbt.ca>

PGP Key: http://www.rbt.ca/rbtpub.asc