need more ALTER TABLE guards for typed tables
Started by Peter Eisentrautover 15 years ago3 messages
After some investigation I figured that I need to add two more checks
into the ALTER TABLE code to prevent certain types of direct changes to
typed tables (see attached patch).
But it's not clear to me whether such checks should go into the "Prep"
or the "Exec" phases. Prep seems more plausible to me, but some
commands such as DropColumn don't have a Prep handler. A clarification
would be helpful.
Attachments:
dont-alter-typed-tables.patchtext/x-patch; charset=UTF-8; name=dont-alter-typed-tables.patchDownload
Index: src/backend/commands/tablecmds.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/tablecmds.c,v
retrieving revision 1.332
diff -u -3 -p -r1.332 tablecmds.c
--- src/backend/commands/tablecmds.c 6 Jul 2010 19:18:56 -0000 1.332
+++ src/backend/commands/tablecmds.c 21 Jul 2010 14:34:41 -0000
@@ -5788,6 +5788,11 @@ ATPrepAlterColumnType(List **wqueue,
NewColumnValue *newval;
ParseState *pstate = make_parsestate(NULL);
+ if (rel->rd_rel->reloftype)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot alter column type of typed table")));
+
/* lookup the attribute so we can check inheritance status */
tuple = SearchSysCacheAttName(RelationGetRelid(rel), colName);
if (!HeapTupleIsValid(tuple))
@@ -7126,6 +7131,11 @@ ATExecAddInherit(Relation child_rel, Ran
int32 inhseqno;
List *children;
+ if (child_rel->rd_rel->reloftype)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot change inheritance of typed table")));
+
/*
* AccessShareLock on the parent is what's obtained during normal CREATE
* TABLE ... INHERITS ..., so should be enough here.
Re: need more ALTER TABLE guards for typed tables
Excerpts from Peter Eisentraut's message of mié jul 21 15:18:58 -0400 2010:
After some investigation I figured that I need to add two more checks
into the ALTER TABLE code to prevent certain types of direct changes to
typed tables (see attached patch).But it's not clear to me whether such checks should go into the "Prep"
or the "Exec" phases. Prep seems more plausible to me, but some
commands such as DropColumn don't have a Prep handler. A clarification
would be helpful.
I think if there's no Prep phase, you should add it. I don't think it
makes sense to have this kind of check in Exec.
Re: need more ALTER TABLE guards for typed tables
On ons, 2010-07-21 at 15:48 -0400, Alvaro Herrera wrote:
Excerpts from Peter Eisentraut's message of mié jul 21 15:18:58 -0400 2010:
After some investigation I figured that I need to add two more checks
into the ALTER TABLE code to prevent certain types of direct changes to
typed tables (see attached patch).But it's not clear to me whether such checks should go into the "Prep"
or the "Exec" phases. Prep seems more plausible to me, but some
commands such as DropColumn don't have a Prep handler. A clarification
would be helpful.I think if there's no Prep phase, you should add it. I don't think it
makes sense to have this kind of check in Exec.
OK, here is my patch for this. (should go into 9.0 and 9.1)
Attachments:
dont-alter-typed-tables.patchtext/x-patch; charset=UTF-8; name=dont-alter-typed-tables.patchDownload
Index: src/backend/commands/tablecmds.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/tablecmds.c,v
retrieving revision 1.332
diff -u -3 -p -r1.332 tablecmds.c
--- src/backend/commands/tablecmds.c 6 Jul 2010 19:18:56 -0000 1.332
+++ src/backend/commands/tablecmds.c 22 Jul 2010 14:53:24 -0000
@@ -288,6 +288,7 @@ static void ATExecSetOptions(Relation re
Node *options, bool isReset);
static void ATExecSetStorage(Relation rel, const char *colName,
Node *newValue);
+static void ATPrepDropColumn(Relation rel, bool recurse, AlterTableCmd *cmd);
static void ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
DropBehavior behavior,
bool recurse, bool recursing,
@@ -327,7 +328,8 @@ static void ATExecEnableDisableTrigger(R
char fires_when, bool skip_system);
static void ATExecEnableDisableRule(Relation rel, char *rulename,
char fires_when);
-static void ATExecAddInherit(Relation rel, RangeVar *parent);
+static void ATPrepAddInherit(Relation child_rel);
+static void ATExecAddInherit(Relation child_rel, RangeVar *parent);
static void ATExecDropInherit(Relation rel, RangeVar *parent);
static void copy_relation_data(SMgrRelation rel, SMgrRelation dst,
ForkNumber forkNum, bool istemp);
@@ -2499,10 +2501,8 @@ ATPrepCmd(List **wqueue, Relation rel, A
break;
case AT_DropColumn: /* DROP COLUMN */
ATSimplePermissions(rel, false);
+ ATPrepDropColumn(rel, recurse, cmd);
/* Recursion occurs during execution phase */
- /* No command-specific prep needed except saving recurse flag */
- if (recurse)
- cmd->subtype = AT_DropColumnRecurse;
pass = AT_PASS_DROP;
break;
case AT_AddIndex: /* ADD INDEX */
@@ -2579,6 +2579,12 @@ ATPrepCmd(List **wqueue, Relation rel, A
/* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
+ case AT_AddInherit: /* INHERIT */
+ ATSimplePermissions(rel, false);
+ /* This command never recurses */
+ ATPrepAddInherit(rel);
+ pass = AT_PASS_MISC;
+ break;
case AT_EnableTrig: /* ENABLE TRIGGER variants */
case AT_EnableAlwaysTrig:
case AT_EnableReplicaTrig:
@@ -2591,8 +2597,7 @@ ATPrepCmd(List **wqueue, Relation rel, A
case AT_EnableAlwaysRule:
case AT_EnableReplicaRule:
case AT_DisableRule:
- case AT_AddInherit: /* INHERIT / NO INHERIT */
- case AT_DropInherit:
+ case AT_DropInherit: /* NO INHERIT */
ATSimplePermissions(rel, false);
/* These commands never recurse */
/* No command-specific prep needed */
@@ -3568,6 +3573,11 @@ static void
ATPrepAddColumn(List **wqueue, Relation rel, bool recurse,
AlterTableCmd *cmd)
{
+ if (rel->rd_rel->reloftype)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot add column to typed table")));
+
/*
* Recurse to add the column to child classes, if requested.
*
@@ -3616,11 +3626,6 @@ ATExecAddColumn(AlteredTableInfo *tab, R
Form_pg_type tform;
Expr *defval;
- if (rel->rd_rel->reloftype)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot add column to typed table")));
-
attrdesc = heap_open(AttributeRelationId, RowExclusiveLock);
/*
@@ -4326,6 +4331,19 @@ ATExecSetStorage(Relation rel, const cha
* correctly.)
*/
static void
+ATPrepDropColumn(Relation rel, bool recurse, AlterTableCmd *cmd)
+{
+ if (rel->rd_rel->reloftype)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot drop column from typed table")));
+
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_DropColumnRecurse;
+}
+
+static void
ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
DropBehavior behavior,
bool recurse, bool recursing,
@@ -4337,11 +4355,6 @@ ATExecDropColumn(List **wqueue, Relation
List *children;
ObjectAddress object;
- if (rel->rd_rel->reloftype)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot drop column from typed table")));
-
/* At top level, permission check was done in ATPrepCmd, else do it */
if (recursing)
ATSimplePermissions(rel, false);
@@ -5788,6 +5801,11 @@ ATPrepAlterColumnType(List **wqueue,
NewColumnValue *newval;
ParseState *pstate = make_parsestate(NULL);
+ if (rel->rd_rel->reloftype)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot alter column type of typed table")));
+
/* lookup the attribute so we can check inheritance status */
tuple = SearchSysCacheAttName(RelationGetRelid(rel), colName);
if (!HeapTupleIsValid(tuple))
@@ -7116,6 +7134,15 @@ ATExecEnableDisableRule(Relation rel, ch
* same data types and expressions.
*/
static void
+ATPrepAddInherit(Relation child_rel)
+{
+ if (child_rel->rd_rel->reloftype)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot change inheritance of typed table")));
+}
+
+static void
ATExecAddInherit(Relation child_rel, RangeVar *parent)
{
Relation parent_rel,
Index: src/test/regress/expected/typed_table.out
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/expected/typed_table.out,v
retrieving revision 1.1
diff -u -3 -p -r1.1 typed_table.out
--- src/test/regress/expected/typed_table.out 28 Jan 2010 23:21:13 -0000 1.1
+++ src/test/regress/expected/typed_table.out 22 Jul 2010 14:53:26 -0000
@@ -25,12 +25,18 @@ SELECT * FROM get_all_persons();
----+------
(0 rows)
+-- certain ALTER TABLE operations on typed tables are not allowed
ALTER TABLE persons ADD COLUMN comment text;
ERROR: cannot add column to typed table
ALTER TABLE persons DROP COLUMN name;
ERROR: cannot drop column from typed table
ALTER TABLE persons RENAME COLUMN id TO num;
ERROR: cannot rename column of typed table
+ALTER TABLE persons ALTER COLUMN name TYPE varchar;
+ERROR: cannot alter column type of typed table
+CREATE TABLE stuff (id int);
+ALTER TABLE persons INHERIT stuff;
+ERROR: cannot change inheritance of typed table
CREATE TABLE personsx OF person_type (myname WITH OPTIONS NOT NULL); -- error
ERROR: column "myname" does not exist
CREATE TABLE persons2 OF person_type (
@@ -83,3 +89,4 @@ DETAIL: drop cascades to table persons
drop cascades to function get_all_persons()
drop cascades to table persons2
drop cascades to table persons3
+DROP TABLE stuff;
Index: src/test/regress/sql/typed_table.sql
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/sql/typed_table.sql,v
retrieving revision 1.1
diff -u -3 -p -r1.1 typed_table.sql
--- src/test/regress/sql/typed_table.sql 28 Jan 2010 23:21:13 -0000 1.1
+++ src/test/regress/sql/typed_table.sql 22 Jul 2010 14:53:26 -0000
@@ -13,9 +13,13 @@ $$;
SELECT * FROM get_all_persons();
+-- certain ALTER TABLE operations on typed tables are not allowed
ALTER TABLE persons ADD COLUMN comment text;
ALTER TABLE persons DROP COLUMN name;
ALTER TABLE persons RENAME COLUMN id TO num;
+ALTER TABLE persons ALTER COLUMN name TYPE varchar;
+CREATE TABLE stuff (id int);
+ALTER TABLE persons INHERIT stuff;
CREATE TABLE personsx OF person_type (myname WITH OPTIONS NOT NULL); -- error
@@ -40,3 +44,5 @@ CREATE TABLE persons4 OF person_type (
DROP TYPE person_type RESTRICT;
DROP TYPE person_type CASCADE;
+
+DROP TABLE stuff;