On Thu, Apr 14, 2011 at 9:38 PM, Noah Misch <noah@leadboat.com> wrote:
> On Thu, Apr 14, 2011 at 11:23:49AM -0700, Robert Haas wrote:
>> On Thu, Apr 14, 2011 at 5:18 AM, Noah Misch <noah@leadboat.com> wrote:
>> >> I guess my gut feeling is that it would make more sense to forbid it
>> >> outright for 9.1, and we can look at relaxing that restriction later
>> >> if we're so inclined.
>> >>
>> >> Much as with the problem Tom fixed in commit
>> >> eb51af71f241e8cb199790dee9ad246bb36b3287, I'm concerned that there may
>> >> be other cases that we're not thinking of right now, and while we
>> >> could find them all and fix them, the amount of functionality gained
>> >> is fairly marginal, and I don't really want to hold up the release
>> >> while we bug-swat.
>> >
>> > Symmetry was the best cause I could find to continue allowing it, and your case
>> > in favor of reducing the bug surface is more compelling. ?Let's forbid it.
>>
>> OK. Care to propose a patch?
>
> Sure; attached. It requires that the type relation be RELKIND_COMPOSITE_TYPE.
> We hadn't explicitly discussed the use of foreign table, view, toast table, or
> sequence row types. The first two might have some value, someday; I'm sure
> nobody cares for the second two.
It appears that this patch - which I've now committed - actually fixes
two bugs. Without the patch, this leaves the tables out of step,
because no relation lock is taken:
S1: CREATE TYPE t AS (a int);
S1: BEGIN;
S1: CREATE TABLE t1 OF t;
S2: ALTER TYPE t ADD ATTRIBUTE b int CASCADE;
S1: COMMIT;
I tweaked the comments accordingly, and also reverted your change to
the error message, because I don't want to introduce new terminology
here that we're not using anywhere else.
I also thought about making this use parserOpenTable() rather than
relation_open(), but it turns out that's not so easy, because the
typed table name is getting shoved into a TypeName rather than a
RangeVar. I don't think that's the choice I would have made but I'm
not sure how excited it's worth getting about it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company