Thread: Typed table DDL loose ends
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
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
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
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
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
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
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.
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
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
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
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
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
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.
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"?
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.
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.
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
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
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.