On Thu, Mar 31, 2011 at 11:11:49AM -0400, Robert Haas wrote:
> On Thu, Mar 31, 2011 at 6:06 AM, Noah Misch <n...@leadboat.com> wrote:
> >> I think this is a manifestation the same problem mentioned here:
> >>
> >> http://git.postgresql.org/gitweb?p=postgresql.git;a=commit;h=31b6fc06d83c6de3644c8f2921eb7de0eb92fac3
> >>
> >> I believe this requires some refactoring to fix. ?It would be good to do
> >> that.
> >
> > The best way I can see is to make ATExecAddColumn more like
> > ATExecDropColumn,
> > ATAddCheckConstraint, and ATExecDropConstraint. ?Namely, recurse at
> > Exec-time
> > rather than Prep-time, and cease recursing when we satisfy the ADD COLUMN
> > with a
> > merge. ?Did you have something else in mind?
>
> I had exactly what you just said in mind.
Patch attached, then.
> > Incidentally, when we satisfy an ADD COLUMN with a merge, we do not check or
> > update attnotnull:
<details ... what should we do?>
> Not sure. I think that anything we do here is bound to have some
> corner cases that are not quite right for so long as NOT NULL
> constraints aren't represented in pg_constraint, and it's way too late
> to dredge up that issue again for 9.1. I'm somewhat inclined to just
> defer fixing it until we get that work committed.
OK.
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 737ab1a..c64ffac 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 285,300 **** static void ATSimplePermissions(Relation rel, int
allowed_targets);
static void ATWrongRelkindError(Relation rel, int allowed_targets);
static void ATSimpleRecursion(List **wqueue, Relation rel,
AlterTableCmd *cmd, bool recurse, LOCKMODE
lockmode);
- static void ATOneLevelRecursion(List **wqueue, Relation rel,
- AlterTableCmd *cmd, LOCKMODE lockmode);
static void ATTypedTableRecursion(List **wqueue, Relation rel, AlterTableCmd
*cmd,
LOCKMODE
lockmode);
static List *find_typed_table_dependencies(Oid typeOid, const char *typeName,
DropBehavior behavior);
static void ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, bool
recursing,
AlterTableCmd *cmd, LOCKMODE lockmode);
! static void ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
! ColumnDef *colDef, bool isOid, LOCKMODE
lockmode);
static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid
typid, Oid collid);
static void ATPrepAddOids(List **wqueue, Relation rel, bool recurse,
AlterTableCmd *cmd, LOCKMODE lockmode);
--- 285,299 ----
static void ATWrongRelkindError(Relation rel, int allowed_targets);
static void ATSimpleRecursion(List **wqueue, Relation rel,
AlterTableCmd *cmd, bool recurse, LOCKMODE
lockmode);
static void ATTypedTableRecursion(List **wqueue, Relation rel, AlterTableCmd
*cmd,
LOCKMODE
lockmode);
static List *find_typed_table_dependencies(Oid typeOid, const char *typeName,
DropBehavior behavior);
static void ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, bool
recursing,
AlterTableCmd *cmd, LOCKMODE lockmode);
! static void ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation
rel,
! ColumnDef *colDef, bool isOid,
! bool recurse, bool recursing, LOCKMODE
lockmode);
static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid
typid, Oid collid);
static void ATPrepAddOids(List **wqueue, Relation rel, bool recurse,
AlterTableCmd *cmd, LOCKMODE lockmode);
***************
*** 2775,2789 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
case AT_AddColumn: /* ADD COLUMN */
ATSimplePermissions(rel,
ATT_TABLE|ATT_COMPOSITE_TYPE|ATT_FOREIGN_TABLE);
- /* Performs own recursion */
ATPrepAddColumn(wqueue, rel, recurse, recursing, cmd,
lockmode);
pass = AT_PASS_ADD_COL;
break;
case AT_AddColumnToView: /* add column via
CREATE OR REPLACE
* VIEW */
ATSimplePermissions(rel, ATT_VIEW);
- /* Performs own recursion */
ATPrepAddColumn(wqueue, rel, recurse, recursing, cmd,
lockmode);
pass = AT_PASS_ADD_COL;
break;
case AT_ColumnDefault: /* ALTER COLUMN DEFAULT */
--- 2774,2788 ----
case AT_AddColumn: /* ADD COLUMN */
ATSimplePermissions(rel,
ATT_TABLE|ATT_COMPOSITE_TYPE|ATT_FOREIGN_TABLE);
ATPrepAddColumn(wqueue, rel, recurse, recursing, cmd,
lockmode);
+ /* Recursion occurs during execution phase */
pass = AT_PASS_ADD_COL;
break;
case AT_AddColumnToView: /* add column via
CREATE OR REPLACE
* VIEW */
ATSimplePermissions(rel, ATT_VIEW);
ATPrepAddColumn(wqueue, rel, recurse, recursing, cmd,
lockmode);
+ /* Recursion occurs during execution phase */
pass = AT_PASS_ADD_COL;
break;
case AT_ColumnDefault: /* ALTER COLUMN DEFAULT */
***************
*** 2885,2893 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_AddOids: /* SET WITH OIDS */
ATSimplePermissions(rel, ATT_TABLE);
- /* Performs own recursion */
if (!rel->rd_rel->relhasoids || recursing)
ATPrepAddOids(wqueue, rel, recurse, cmd,
lockmode);
pass = AT_PASS_ADD_COL;
break;
case AT_DropOids: /* SET WITHOUT OIDS */
--- 2884,2892 ----
break;
case AT_AddOids: /* SET WITH OIDS */
ATSimplePermissions(rel, ATT_TABLE);
if (!rel->rd_rel->relhasoids || recursing)
ATPrepAddOids(wqueue, rel, recurse, cmd,
lockmode);
+ /* Recursion occurs during execution phase */
pass = AT_PASS_ADD_COL;
break;
case AT_DropOids: /* SET WITHOUT OIDS */
***************
*** 3043,3049 **** ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
case AT_AddColumn: /* ADD COLUMN */
case AT_AddColumnToView: /* add column via
CREATE OR REPLACE
* VIEW */
! ATExecAddColumn(tab, rel, (ColumnDef *) cmd->def,
false, lockmode);
break;
case AT_ColumnDefault: /* ALTER COLUMN DEFAULT */
ATExecColumnDefault(rel, cmd->name, cmd->def, lockmode);
--- 3042,3053 ----
case AT_AddColumn: /* ADD COLUMN */
case AT_AddColumnToView: /* add column via
CREATE OR REPLACE
* VIEW */
! ATExecAddColumn(wqueue, tab, rel, (ColumnDef *)
cmd->def,
! false, false, false,
lockmode);
! break;
! case AT_AddColumnRecurse:
! ATExecAddColumn(wqueue, tab, rel, (ColumnDef *)
cmd->def,
! false, true, false,
lockmode);
break;
case AT_ColumnDefault: /* ALTER COLUMN DEFAULT */
ATExecColumnDefault(rel, cmd->name, cmd->def, lockmode);
***************
*** 3121,3127 **** ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
case AT_AddOids: /* SET WITH OIDS */
/* Use the ADD COLUMN code, unless prep decided to do
nothing */
if (cmd->def != NULL)
! ATExecAddColumn(tab, rel, (ColumnDef *)
cmd->def, true, lockmode);
break;
case AT_DropOids: /* SET WITHOUT OIDS */
--- 3125,3138 ----
case AT_AddOids: /* SET WITH OIDS */
/* Use the ADD COLUMN code, unless prep decided to do
nothing */
if (cmd->def != NULL)
! ATExecAddColumn(wqueue, tab, rel, (ColumnDef *)
cmd->def,
! true, false,
false, lockmode);
! break;
! case AT_AddOidsRecurse: /* SET WITH OIDS */
! /* Use the ADD COLUMN code, unless prep decided to do
nothing */
! if (cmd->def != NULL)
! ATExecAddColumn(wqueue, tab, rel, (ColumnDef *)
cmd->def,
! true, true,
false, lockmode);
break;
case AT_DropOids: /* SET WITHOUT OIDS */
***************
*** 3843,3879 **** ATSimpleRecursion(List **wqueue, Relation rel,
}
/*
- * ATOneLevelRecursion
- *
- * Here, we visit only direct inheritance children. It is expected that
- * the command's prep routine will recurse again to find indirect children.
- * When using this technique, a multiply-inheriting child will be visited
- * multiple times.
- */
- static void
- ATOneLevelRecursion(List **wqueue, Relation rel,
- AlterTableCmd *cmd, LOCKMODE lockmode)
- {
- Oid relid = RelationGetRelid(rel);
- ListCell *child;
- List *children;
-
- children = find_inheritance_children(relid, lockmode);
-
- foreach(child, children)
- {
- Oid childrelid = lfirst_oid(child);
- Relation childrel;
-
- /* find_inheritance_children already got lock */
- childrel = relation_open(childrelid, NoLock);
- CheckTableNotInUse(childrel, "ALTER TABLE");
- ATPrepCmd(wqueue, childrel, cmd, true, true, lockmode);
- relation_close(childrel, NoLock);
- }
- }
-
- /*
* ATTypedTableRecursion
*
* Propagate ALTER TYPE operations to the typed tables of that type.
--- 3854,3859 ----
***************
*** 4060,4065 **** find_typed_table_dependencies(Oid typeOid, const char
*typeName, DropBehavior be
--- 4040,4051 ----
* CHECK, NOT NULL, and FOREIGN KEY constraints will be removed from the
* AT_AddColumn AlterTableCmd by parse_utilcmd.c and added as independent
* AlterTableCmd's.
+ *
+ * ADD COLUMN cannot use the normal ALTER TABLE recursion mechanism, because
we
+ * have to decide at runtime whether to recurse or not depending on whether we
+ * actually add a column or merely merge with an existing column. (We can't
+ * check this in a static pre-pass because it won't handle multiple
inheritance
+ * situations correctly.)
*/
static void
ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, bool recursing,
***************
*** 4070,4112 **** ATPrepAddColumn(List **wqueue, Relation rel, bool recurse,
bool recursing,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot add column to typed table")));
- /*
- * Recurse to add the column to child classes, if requested.
- *
- * We must recurse one level at a time, so that multiply-inheriting
- * children are visited the right number of times and end up with the
- * right attinhcount.
- */
- if (recurse)
- {
- AlterTableCmd *childCmd = copyObject(cmd);
- ColumnDef *colDefChild = (ColumnDef *) childCmd->def;
-
- /* Child should see column as singly inherited */
- colDefChild->inhcount = 1;
- colDefChild->is_local = false;
-
- ATOneLevelRecursion(wqueue, rel, childCmd, lockmode);
- }
- else
- {
- /*
- * If we are told not to recurse, there had better not be any
child
- * tables; else the addition would put them out of step.
- */
- if (find_inheritance_children(RelationGetRelid(rel), NoLock) !=
NIL)
- ereport(ERROR,
-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("column must be added to child
tables too")));
- }
-
if (rel->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
ATTypedTableRecursion(wqueue, rel, cmd, lockmode);
}
static void
! ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
! ColumnDef *colDef, bool isOid, LOCKMODE
lockmode)
{
Oid myrelid = RelationGetRelid(rel);
Relation pgclass,
--- 4056,4072 ----
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot add column to typed table")));
if (rel->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
ATTypedTableRecursion(wqueue, rel, cmd, lockmode);
+
+ if (recurse)
+ cmd->subtype = AT_AddColumnRecurse;
}
static void
! ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
! ColumnDef *colDef, bool isOid,
! bool recurse, bool recursing, LOCKMODE lockmode)
{
Oid myrelid = RelationGetRelid(rel);
Relation pgclass,
***************
*** 4121,4132 **** ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
Oid collOid;
Form_pg_type tform;
Expr *defval;
attrdesc = heap_open(AttributeRelationId, RowExclusiveLock);
/*
* Are we adding the column to a recursion child? If so, check whether
to
! * merge with an existing definition for the column.
*/
if (colDef->inhcount > 0)
{
--- 4081,4099 ----
Oid collOid;
Form_pg_type tform;
Expr *defval;
+ List *children;
+ ListCell *child;
+
+ /* At top level, permission check was done in ATPrepCmd, else do it */
+ if (recursing)
+ ATSimplePermissions(rel, ATT_TABLE);
attrdesc = heap_open(AttributeRelationId, RowExclusiveLock);
/*
* Are we adding the column to a recursion child? If so, check whether
to
! * merge with an existing definition for the column. If we do merge,
! * there's also no need to recurse. Children will already have the
column.
*/
if (colDef->inhcount > 0)
{
***************
*** 4389,4394 **** ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
--- 4356,4405 ----
* Add needed dependency entries for the new column.
*/
add_column_datatype_dependency(myrelid, newattnum, attribute.atttypid,
attribute.attcollation);
+
+ /*
+ * Propagate to children as appropriate. Unlike most other ALTER
+ * routines, we have to do this one level of recursion at a time; we
can't
+ * use find_all_inheritors to do it in one pass.
+ */
+ children = find_inheritance_children(RelationGetRelid(rel), lockmode);
+
+ /*
+ * If we are told not to recurse, there had better not be any child
+ * tables; else the addition would put them out of step.
+ */
+ if (children && !recurse)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("column must be added to child tables
too")));
+
+ /* Children should see column as singly inherited */
+ if (!recursing)
+ {
+ colDef = copyObject(colDef);
+ colDef->inhcount = 1;
+ colDef->is_local = false;
+ }
+
+ foreach(child, children)
+ {
+ Oid childrelid = lfirst_oid(child);
+ Relation childrel;
+ AlteredTableInfo *childtab;
+
+ /* find_inheritance_children already got lock */
+ childrel = heap_open(childrelid, NoLock);
+ CheckTableNotInUse(childrel, "ALTER TABLE");
+
+ /* Find or create work queue entry for this table */
+ childtab = ATGetQueueEntry(wqueue, childrel);
+
+ /* Recurse to child */
+ ATExecAddColumn(wqueue, childtab, childrel,
+ colDef, isOid, recurse, true,
lockmode);
+
+ heap_close(childrel, NoLock);
+ }
}
/*
***************
*** 4440,4445 **** ATPrepAddOids(List **wqueue, Relation rel, bool recurse,
AlterTableCmd *cmd, LOC
--- 4451,4459 ----
cmd->def = (Node *) cdef;
}
ATPrepAddColumn(wqueue, rel, recurse, false, cmd, lockmode);
+
+ if (recurse)
+ cmd->subtype = AT_AddOidsRecurse;
}
/*
diff --git a/src/include/nodes/parsenodindex 670b12f..d9eac76 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 1173,1178 **** typedef struct AlterTableStmt
--- 1173,1179 ----
typedef enum AlterTableType
{
AT_AddColumn, /* add column */
+ AT_AddColumnRecurse, /* internal to commands/tablecmds.c */
AT_AddColumnToView, /* implicitly via CREATE OR
REPLACE VIEW */
AT_ColumnDefault, /* alter column default */
AT_DropNotNull, /* alter column drop not null */
***************
*** 1198,1203 **** typedef enum AlterTableType
--- 1199,1205 ----
AT_ClusterOn, /* CLUSTER ON */
AT_DropCluster, /* SET WITHOUT CLUSTER */
AT_AddOids, /* SET WITH OIDS */
+ AT_AddOidsRecurse, /* internal to
commands/tablecmds.c */
AT_DropOids, /* SET WITHOUT OIDS */
AT_SetTableSpace, /* SET TABLESPACE */
AT_SetRelOptions, /* SET (...) -- AM specific
parameters */
diff --git a/src/test/regress/expecteindex 578f94b..d7d1b64 100644
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
***************
*** 1198,1203 **** drop table p1, p2 cascade;
--- 1198,1220 ----
NOTICE: drop cascades to 2 other objects
DETAIL: drop cascades to table c1
drop cascades to table gc1
+ -- test attinhcount tracking with merged columns
+ create table depth0();
+ create table depth1(c text) inherits (depth0);
+ create table depth2() inherits (depth1);
+ alter table depth0 add c text;
+ NOTICE: merging definition of column "c" for child "depth1"
+ select attrelid::regclass, attname, attinhcount, attislocal
+ from pg_attribute
+ where attnum > 0 and attrelid::regclass in ('depth0', 'depth1', 'depth2')
+ order by attrelid::regclass::text, attnum;
+ attrelid | attname | attinhcount | attislocal
+ ----------+---------+-------------+------------
+ depth0 | c | 0 | t
+ depth1 | c | 1 | t
+ depth2 | c | 1 | f
+ (3 rows)
+
--
-- Test the ALTER TABLE SET WITH/WITHOUT OIDS command
--
diff --git a/src/test/regress/sql/alter_table.sqindex 54dbb8e..749584d 100644
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
***************
*** 944,949 **** order by relname, attnum;
--- 944,961 ----
drop table p1, p2 cascade;
+ -- test attinhcount tracking with merged columns
+
+ create table depth0();
+ create table depth1(c text) inherits (depth0);
+ create table depth2() inherits (depth1);
+ alter table depth0 add c text;
+
+ select attrelid::regclass, attname, attinhcount, attislocal
+ from pg_attribute
+ where attnum > 0 and attrelid::regclass in ('depth0', 'depth1', 'depth2')
+ order by attrelid::regclass::text, attnum;
+
--
-- Test the ALTER TABLE SET WITH/WITHOUT OIDS command
--
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
