Thread: Typed table DDL loose ends

Typed table DDL loose ends

From
Noah Misch
Date:
While looking at the typed table/pg_upgrade problem, I ran into a few smaller
problems in the area.  I'm not envisioning a need for much code shift to fix
them, but there are a few points of policy.

* Table row types used in typed tables vs. ALTER TABLE
As previously noted: CREATE TABLE t (); CREATE TABLE is_a OF t; ALTER TABLE t ADD c int; \d is_a -- No columns

At first I thought we should just forbid the use of table row types in CREATE
TABLE OF.  However, we've been quite systematic about not distinguishing between
table row types and CREATE TYPE AS types; I've only found a distinction in ALTER
TABLE/ALTER TYPE, where we direct you to the other command.  It would be nice to
preserve this heritage.  That doesn't look particularly difficult; it may
actually yield a net code reduction.

There is a minor policy question: when should ALTER TABLE behave like ALTER TYPE
... RESTRICT, if ever?  Would using the inheritance recursion decision (driven
by ONLY, *, and sql_inheritance) be sufficiently reasonable, or do we need a
distinct signal?  I can't envision a case where you'd want to recurse to
inheritance children but error on typed table children (YMMV).  ALTER TABLE DROP
COLUMN currently uses RESTRICT/CASCADE in a completely different sense, so any
syntactic signal would need to step around that.

* Inheriting from a typed table blocks further type DDL CREATE TYPE t AS (x int); CREATE TABLE parent OF t; CREATE
TABLEchild () INHERITS (parent); ALTER TYPE t ADD ATTRIBUTE y int CASCADE; -- ERROR:  column must be added to child
tablestoo
 
We ought to just set INH_YES on the downstream command in ATTypedTableRecursion.
If we get to that point, the user did choose ALTER TYPE CASCADE; it seems fair
to assume he'd want inheritance recursion rather than a later error.

* Users can CREATE TABLE OF on a type they don't own
This in turns blocks the owner's ability to alter the table/type.  However, we
already have this hazard with composite-type columns.  A TODO to address this
broadly seems in order, but it's not a 9.1 issue.

* Can create a permanent table using a temp table row type CREATE TEMP TABLE tempt (); CREATE TABLE permt OF tempt; --
silentlydropped on session exit
 
Looks easy to fix, with no policy questions.

Does any of this appear incorrect or unreasonable?

Thanks,
nm


Re: Typed table DDL loose ends

From
Robert Haas
Date:
On Sat, Apr 9, 2011 at 6:57 PM, Noah Misch <noah@leadboat.com> wrote:
> While looking at the typed table/pg_upgrade problem, I ran into a few smaller
> problems in the area.  I'm not envisioning a need for much code shift to fix
> them, but there are a few points of policy.
>
> * Table row types used in typed tables vs. ALTER TABLE
> As previously noted:
>  CREATE TABLE t ();
>  CREATE TABLE is_a OF t;
>  ALTER TABLE t ADD c int;
>  \d is_a
>  -- No columns
>
> At first I thought we should just forbid the use of table row types in CREATE
> TABLE OF.  However, we've been quite systematic about not distinguishing between
> table row types and CREATE TYPE AS types; I've only found a distinction in ALTER
> TABLE/ALTER TYPE, where we direct you to the other command.  It would be nice to
> preserve this heritage.  That doesn't look particularly difficult; it may
> actually yield a net code reduction.

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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Typed table DDL loose ends

From
Noah Misch
Date:
On Wed, Apr 13, 2011 at 07:57:29PM -0700, Robert Haas wrote:
> On Sat, Apr 9, 2011 at 6:57 PM, Noah Misch <noah@leadboat.com> wrote:
> > While looking at the typed table/pg_upgrade problem, I ran into a few smaller
> > problems in the area. ?I'm not envisioning a need for much code shift to fix
> > them, but there are a few points of policy.
> >
> > * Table row types used in typed tables vs. ALTER TABLE
> > As previously noted:
> > ?CREATE TABLE t ();
> > ?CREATE TABLE is_a OF t;
> > ?ALTER TABLE t ADD c int;
> > ?\d is_a
> > ?-- No columns
> >
> > At first I thought we should just forbid the use of table row types in CREATE
> > TABLE OF. ?However, we've been quite systematic about not distinguishing between
> > table row types and CREATE TYPE AS types; I've only found a distinction in ALTER
> > TABLE/ALTER TYPE, where we direct you to the other command. ?It would be nice to
> > preserve this heritage. ?That doesn't look particularly difficult; it may
> > actually yield a net code reduction.
> 
> 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.

Thanks,
nm


Re: Typed table DDL loose ends

From
Robert Haas
Date:
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?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Typed table DDL loose ends

From
Noah Misch
Date:
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.

nm

Attachment

Re: Typed table DDL loose ends

From
Robert Haas
Date:
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


Re: Typed table DDL loose ends

From
Noah Misch
Date:
On Mon, Apr 18, 2011 at 10:20:21AM -0400, Robert Haas wrote:
> 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.

FWIW, the term "stand-alone composite type" appears twice in our documentation.

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

I hadn't thought about this angle.


Re: Typed table DDL loose ends

From
Robert Haas
Date:
On Mon, Apr 18, 2011 at 10:48 AM, Noah Misch <noah@leadboat.com> wrote:
> On Mon, Apr 18, 2011 at 10:20:21AM -0400, Robert Haas wrote:
>> 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.
>
> FWIW, the term "stand-alone composite type" appears twice in our documentation.

Hmm, OK.  Anyone else have an opinion on the relative merits of:

ERROR: type stuff is not a composite type
vs.
ERROR: type stuff is not a stand-alone composite type

The intent of adding "stand-alone" was, I believe, to clarify that it
has to be a CREATE TYPE stuff AS ... type, not just a row type (that
is, naturally, composite, in some less-pure sense).  I'm not sure
whether the extra word actually makes it more clear, though.

Opinions?  Suggestions?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Typed table DDL loose ends

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Apr 18, 2011 at 10:48 AM, Noah Misch <noah@leadboat.com> wrote:
>> FWIW, the term "stand-alone composite type" appears twice in our documentation.

> Hmm, OK.  Anyone else have an opinion on the relative merits of:

> ERROR: type stuff is not a composite type
> vs.
> ERROR: type stuff is not a stand-alone composite type

> The intent of adding "stand-alone" was, I believe, to clarify that it
> has to be a CREATE TYPE stuff AS ... type, not just a row type (that
> is, naturally, composite, in some less-pure sense).  I'm not sure
> whether the extra word actually makes it more clear, though.

In 99.9% of the code and docs, a table rowtype is a perfectly good
composite type.  I agree with Noah that just saying "composite type"
is inadequate here; but I'm not sure that "stand-alone" is a helpful
adjective either.  What about inverting the message phrasing, ie

ERROR: type stuff must not be a table's row type

You might need some extra logic to keep on giving "is not a composite
type" in cases where it's not composite at all.  But this is enough of a
departure from our usual behavior that I think the error message had
better be pretty darn clear.
        regards, tom lane


Re: Typed table DDL loose ends

From
Robert Haas
Date:
On Mon, Apr 18, 2011 at 11:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Apr 18, 2011 at 10:48 AM, Noah Misch <noah@leadboat.com> wrote:
>>> FWIW, the term "stand-alone composite type" appears twice in our documentation.
>
>> Hmm, OK.  Anyone else have an opinion on the relative merits of:
>
>> ERROR: type stuff is not a composite type
>> vs.
>> ERROR: type stuff is not a stand-alone composite type
>
>> The intent of adding "stand-alone" was, I believe, to clarify that it
>> has to be a CREATE TYPE stuff AS ... type, not just a row type (that
>> is, naturally, composite, in some less-pure sense).  I'm not sure
>> whether the extra word actually makes it more clear, though.
>
> In 99.9% of the code and docs, a table rowtype is a perfectly good
> composite type.  I agree with Noah that just saying "composite type"
> is inadequate here; but I'm not sure that "stand-alone" is a helpful
> adjective either.  What about inverting the message phrasing, ie
>
> ERROR: type stuff must not be a table's row type

It also can't be a view's row type, a sequence's row type, a foreign
table's row type...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Typed table DDL loose ends

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Apr 18, 2011 at 11:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What about inverting the message phrasing, ie
>> 
>> ERROR: type stuff must not be a table's row type

> It also can't be a view's row type, a sequence's row type, a foreign
> table's row type...

Well, you could say "relation's row type" if you wanted to be formally
correct, but I'm not convinced that's an improvement.
        regards, tom lane


Re: Typed table DDL loose ends

From
Robert Haas
Date:
On Mon, Apr 18, 2011 at 11:46 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Apr 18, 2011 at 11:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> What about inverting the message phrasing, ie
>>>
>>> ERROR: type stuff must not be a table's row type
>
>> It also can't be a view's row type, a sequence's row type, a foreign
>> table's row type...
>
> Well, you could say "relation's row type" if you wanted to be formally
> correct, but I'm not convinced that's an improvement.

Me neither, especially since composite types are also relations, in
our parlance.

I'm not strongly attached to or repulsed by any particular option, so
however we end up doing it is OK with me.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Typed table DDL loose ends

From
Peter Eisentraut
Date:
On Sat, 2011-04-09 at 21:57 -0400, Noah Misch wrote:
> * Table row types used in typed tables vs. ALTER TABLE

This item was addressed, but the other ones were not, I think.

> * Inheriting from a typed table blocks further type DDL
>   CREATE TYPE t AS (x int);
>   CREATE TABLE parent OF t;
>   CREATE TABLE child () INHERITS (parent);
>   ALTER TYPE t ADD ATTRIBUTE y int CASCADE;
>   -- ERROR:  column must be added to child tables too
> We ought to just set INH_YES on the downstream command in ATTypedTableRecursion.
> If we get to that point, the user did choose ALTER TYPE CASCADE; it seems fair
> to assume he'd want inheritance recursion rather than a later error.

Agreed.

> * Users can CREATE TABLE OF on a type they don't own
> This in turns blocks the owner's ability to alter the table/type.  However, we
> already have this hazard with composite-type columns.  A TODO to address this
> broadly seems in order, but it's not a 9.1 issue.

I think we should change that to mirror the inheritance policy: you have
to be the owner of the "parent".  Note that this is newly relevant in
9.1, because you couldn't change composite types before.

> * Can create a permanent table using a temp table row type
>   CREATE TEMP TABLE tempt ();
>   CREATE TABLE permt OF tempt; -- silently dropped on session exit
> Looks easy to fix, with no policy questions.

This is now prohibited.



Re: Typed table DDL loose ends

From
Noah Misch
Date:
On Mon, Apr 18, 2011 at 10:44:53PM +0300, Peter Eisentraut wrote:
> On Sat, 2011-04-09 at 21:57 -0400, Noah Misch wrote:
> > * Users can CREATE TABLE OF on a type they don't own
> > This in turns blocks the owner's ability to alter the table/type.  However, we
> > already have this hazard with composite-type columns.  A TODO to address this
> > broadly seems in order, but it's not a 9.1 issue.
> 
> I think we should change that to mirror the inheritance policy: you have
> to be the owner of the "parent".  Note that this is newly relevant in
> 9.1, because you couldn't change composite types before.

Would we add that restriction to use of "CREATE TABLE t (c comptype)" as well,
or just to "CREATE TABLE t OF comptype"?


Re: Typed table DDL loose ends

From
Peter Eisentraut
Date:
On Mon, 2011-04-18 at 19:34 -0400, Noah Misch wrote:
> On Mon, Apr 18, 2011 at 10:44:53PM +0300, Peter Eisentraut wrote:
> > On Sat, 2011-04-09 at 21:57 -0400, Noah Misch wrote:
> > > * Users can CREATE TABLE OF on a type they don't own
> > > This in turns blocks the owner's ability to alter the table/type.  However, we
> > > already have this hazard with composite-type columns.  A TODO to address this
> > > broadly seems in order, but it's not a 9.1 issue.
> > 
> > I think we should change that to mirror the inheritance policy: you have
> > to be the owner of the "parent".  Note that this is newly relevant in
> > 9.1, because you couldn't change composite types before.
> 
> Would we add that restriction to use of "CREATE TABLE t (c comptype)" as well,
> or just to "CREATE TABLE t OF comptype"?

Well, that's a tough one.  It would be a regression.  I would say no to
changing the former, because the fact that this prevents the type owner
from changing the type is not a regression because you couldn't change
types before at all.

Probably we need some privileges on types to address this properly.




Re: Typed table DDL loose ends

From
Noah Misch
Date:
On Wed, Apr 20, 2011 at 12:26:01AM +0300, Peter Eisentraut wrote:
> On Mon, 2011-04-18 at 19:34 -0400, Noah Misch wrote:
> > On Mon, Apr 18, 2011 at 10:44:53PM +0300, Peter Eisentraut wrote:
> > > On Sat, 2011-04-09 at 21:57 -0400, Noah Misch wrote:
> > > > * Users can CREATE TABLE OF on a type they don't own
> > > > This in turns blocks the owner's ability to alter the table/type.  However, we
> > > > already have this hazard with composite-type columns.  A TODO to address this
> > > > broadly seems in order, but it's not a 9.1 issue.
> > > 
> > > I think we should change that to mirror the inheritance policy: you have
> > > to be the owner of the "parent".  Note that this is newly relevant in
> > > 9.1, because you couldn't change composite types before.

I pondered this angle further.  The restriction is critical for inheritance,
because attaching a new child table changes query behavior for the parent.
Typed tables don't have that kind of action at a distance.

> > Would we add that restriction to use of "CREATE TABLE t (c comptype)" as well,
> > or just to "CREATE TABLE t OF comptype"?
> 
> Well, that's a tough one.  It would be a regression.  I would say no to
> changing the former, because the fact that this prevents the type owner
> from changing the type is not a regression because you couldn't change
> types before at all.

In 9.0, an unprivileged second party can use a composite-typed column to block
"DROP TYPE".  If we change nothing, 9.1 will additionally permit blocking "ALTER
TYPE ALTER ATTRIBUTE", "ALTER TYPE ADD ATTRIBUTE" and "ALTER TYPE DROP
ATTRIBUTE".  As you say, those are all new features.  Adding an ownership check
to CREATE TABLE OF would remove the last two vulnerabilities; a composite-typed
column does not block them, but a typed table does block them.

If we add that ownership check, we'll protect some operations on the type.  The
cost is localized divergence from our principle that types have no usage
restrictions.  I'm of the opinion that it's not worth introducing that policy
exception to block just some of these avenues of attack.  I would not object to
it, though.

> Probably we need some privileges on types to address this properly.

Agreed.


Re: Typed table DDL loose ends

From
Noah Misch
Date:
On Mon, Apr 18, 2011 at 10:44:53PM +0300, Peter Eisentraut wrote:
> On Sat, 2011-04-09 at 21:57 -0400, Noah Misch wrote:
> > * Inheriting from a typed table blocks further type DDL
> >   CREATE TYPE t AS (x int);
> >   CREATE TABLE parent OF t;
> >   CREATE TABLE child () INHERITS (parent);
> >   ALTER TYPE t ADD ATTRIBUTE y int CASCADE;
> >   -- ERROR:  column must be added to child tables too
> > We ought to just set INH_YES on the downstream command in ATTypedTableRecursion.
> > If we get to that point, the user did choose ALTER TYPE CASCADE; it seems fair
> > to assume he'd want inheritance recursion rather than a later error.
>
> Agreed.

Patch attached for that.  Apart from a comment, a test case and a doc update, it
turned out to be a one-liner.

Attachment

Re: Typed table DDL loose ends

From
Robert Haas
Date:
On Wed, Apr 20, 2011 at 10:53 AM, Noah Misch <noah@leadboat.com> wrote:
> On Mon, Apr 18, 2011 at 10:44:53PM +0300, Peter Eisentraut wrote:
>> On Sat, 2011-04-09 at 21:57 -0400, Noah Misch wrote:
>> > * Inheriting from a typed table blocks further type DDL
>> >   CREATE TYPE t AS (x int);
>> >   CREATE TABLE parent OF t;
>> >   CREATE TABLE child () INHERITS (parent);
>> >   ALTER TYPE t ADD ATTRIBUTE y int CASCADE;
>> >   -- ERROR:  column must be added to child tables too
>> > We ought to just set INH_YES on the downstream command in ATTypedTableRecursion.
>> > If we get to that point, the user did choose ALTER TYPE CASCADE; it seems fair
>> > to assume he'd want inheritance recursion rather than a later error.
>>
>> Agreed.
>
> Patch attached for that.  Apart from a comment, a test case and a doc update, it
> turned out to be a one-liner.

I have committed this.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: Typed table DDL loose ends

From
Peter Eisentraut
Date:
On Wed, 2011-04-20 at 10:44 -0400, Noah Misch wrote:
> If we add that ownership check, we'll protect some operations on the
> type.  The
> cost is localized divergence from our principle that types have no
> usage
> restrictions.  I'm of the opinion that it's not worth introducing that
> policy
> exception to block just some of these avenues of attack.  I would not
> object to
> it, though. 

So that means we should leave it as is for now?  Fine with me.