> I had a deeper look at this now. The patch looks clean and applies without > any problems, regression tests passes. However, ATRewriteTables() has a > problem when adding columns with domains and constraints. Consider this > small test case: > > CREATE TABLE bar (id INTEGER); > CREATE OR REPLACE VIEW vbar AS SELECT * FROM bar; > CREATE DOMAIN person AS TEXT CHECK(value IN ('haas', 'helmle')); > ALTER TABLE bar ADD COLUMN name person; > CREATE OR REPLACE VIEW vbar AS SELECT * FROM bar; > > The last command confuses ATRewriteTable(), which wants to scan the relation > leading to this error: > ERROR: could not open relation base/16384/16476: > > I see that ATRewriteTable() errors out on heap_beginscan(), since needscan > is set to TRUE. One solution would be to teach ATRewriteTable(s) to handle > view alteration differently in this case.
After looking at this, I think the root cause of this problem is that ATPrepAddColumn isn't smart enough to know that when the underlying relation is a view, there's no point in asking for a table rewrite. Please find an updated patch that addresses this problem. Thanks again for the review - let me know what you think of this version! ...Robert
Index: doc/src/sgml/ref/create_view.sgml =================================================================== RCS file: /projects/cvsroot/pgsql/doc/src/sgml/ref/create_view.sgml,v retrieving revision 1.37 diff -c -r1.37 create_view.sgml *** doc/src/sgml/ref/create_view.sgml 14 Nov 2008 10:22:46 -0000 1.37 --- doc/src/sgml/ref/create_view.sgml 2 Dec 2008 01:39:07 -0000 *************** *** 37,45 **** <para> <command>CREATE OR REPLACE VIEW</command> is similar, but if a view ! of the same name already exists, it is replaced. You can only replace ! a view with a new query that generates the identical set of columns ! (i.e., same column names and data types). </para> <para> --- 37,46 ---- <para> <command>CREATE OR REPLACE VIEW</command> is similar, but if a view ! of the same name already exists, it is replaced. The new query must ! generate all of the same columns that were generated by the original query ! in the same order and with the same data types, but may add additional ! columns to the end of the list. </para> <para> Index: src/backend/commands/tablecmds.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/backend/commands/tablecmds.c,v retrieving revision 1.271 diff -c -r1.271 tablecmds.c *** src/backend/commands/tablecmds.c 19 Nov 2008 10:34:51 -0000 1.271 --- src/backend/commands/tablecmds.c 2 Dec 2008 01:39:12 -0000 *************** *** 2334,2339 **** --- 2334,2345 ---- ATPrepAddColumn(wqueue, rel, recurse, cmd); pass = AT_PASS_ADD_COL; break; + case AT_AddColumnToView: /* add column via CREATE OR REPLACE VIEW */ + ATSimplePermissions(rel, true); + /* Performs own recursion */ + ATPrepAddColumn(wqueue, rel, recurse, cmd); + pass = AT_PASS_ADD_COL; + break; case AT_ColumnDefault: /* ALTER COLUMN DEFAULT */ /* *************** *** 2555,2560 **** --- 2561,2567 ---- switch (cmd->subtype) { case AT_AddColumn: /* ADD COLUMN */ + case AT_AddColumnToView: /* add column via CREATE OR REPLACE VIEW */ ATExecAddColumn(tab, rel, (ColumnDef *) cmd->def); break; case AT_ColumnDefault: /* ALTER COLUMN DEFAULT */ *************** *** 3455,3460 **** --- 3462,3468 ---- int i; int minattnum, maxatts; + char relkind; HeapTuple typeTuple; Oid typeOid; int32 typmod; *************** *** 3527,3532 **** --- 3535,3541 ---- colDef->colname, RelationGetRelationName(rel)))); minattnum = ((Form_pg_class) GETSTRUCT(reltup))->relnatts; + relkind = ((Form_pg_class) GETSTRUCT(reltup))->relkind; maxatts = minattnum + 1; if (maxatts > MaxHeapAttributeNumber) ereport(ERROR, *************** *** 3625,3668 **** * Note: we use build_column_default, and not just the cooked default * returned by AddRelationNewConstraints, so that the right thing happens * when a datatype's default applies. */ ! defval = (Expr *) build_column_default(rel, attribute.attnum); ! if (!defval && GetDomainConstraints(typeOid) != NIL) ! { ! Oid baseTypeId; ! int32 baseTypeMod; ! baseTypeMod = typmod; ! baseTypeId = getBaseTypeAndTypmod(typeOid, &baseTypeMod); ! defval = (Expr *) makeNullConst(baseTypeId, baseTypeMod); ! defval = (Expr *) coerce_to_target_type(NULL, ! (Node *) defval, ! baseTypeId, ! typeOid, ! typmod, ! COERCION_ASSIGNMENT, ! COERCE_IMPLICIT_CAST, ! -1); ! if (defval == NULL) /* should not happen */ ! elog(ERROR, "failed to coerce base type to domain"); ! } ! if (defval) ! { ! NewColumnValue *newval; ! newval = (NewColumnValue *) palloc0(sizeof(NewColumnValue)); ! newval->attnum = attribute.attnum; ! newval->expr = defval; ! tab->newvals = lappend(tab->newvals, newval); ! } ! /* ! * If the new column is NOT NULL, tell Phase 3 it needs to test that. ! */ ! tab->new_notnull |= colDef->is_not_null; /* * Add needed dependency entries for the new column. --- 3634,3681 ---- * Note: we use build_column_default, and not just the cooked default * returned by AddRelationNewConstraints, so that the right thing happens * when a datatype's default applies. + * + * We skip this logic completely for views. */ ! if (relkind != RELKIND_VIEW) { ! defval = (Expr *) build_column_default(rel, attribute.attnum); ! if (!defval && GetDomainConstraints(typeOid) != NIL) ! { ! Oid baseTypeId; ! int32 baseTypeMod; ! baseTypeMod = typmod; ! baseTypeId = getBaseTypeAndTypmod(typeOid, &baseTypeMod); ! defval = (Expr *) makeNullConst(baseTypeId, baseTypeMod); ! defval = (Expr *) coerce_to_target_type(NULL, ! (Node *) defval, ! baseTypeId, ! typeOid, ! typmod, ! COERCION_ASSIGNMENT, ! COERCE_IMPLICIT_CAST, ! -1); ! if (defval == NULL) /* should not happen */ ! elog(ERROR, "failed to coerce base type to domain"); ! } ! if (defval) ! { ! NewColumnValue *newval; ! newval = (NewColumnValue *) palloc0(sizeof(NewColumnValue)); ! newval->attnum = attribute.attnum; ! newval->expr = defval; ! tab->newvals = lappend(tab->newvals, newval); ! } ! /* ! * If the new column is NOT NULL, tell Phase 3 it needs to test that. ! */ ! tab->new_notnull |= colDef->is_not_null; ! } /* * Add needed dependency entries for the new column. Index: src/backend/commands/view.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/backend/commands/view.c,v retrieving revision 1.107 diff -c -r1.107 view.c *** src/backend/commands/view.c 25 Aug 2008 22:42:32 -0000 1.107 --- src/backend/commands/view.c 2 Dec 2008 01:39:12 -0000 *************** *** 173,180 **** Assert(relation->istemp == rel->rd_istemp); /* * Create a tuple descriptor to compare against the existing view, and ! * verify it matches. */ descriptor = BuildDescForRelation(attrList); checkViewTupleDesc(descriptor, rel->rd_att); --- 173,205 ---- Assert(relation->istemp == rel->rd_istemp); /* + * If new attributes have been added, we must modify the pre-existing + * view. + */ + if (list_length(attrList) > rel->rd_att->natts) { + List *atcmds = NIL; + ListCell *c; + int skip = rel->rd_att->natts; + + foreach(c, attrList) { + AlterTableCmd *atcmd; + + if (skip > 0) { + --skip; + continue; + } + atcmd = makeNode(AlterTableCmd); + atcmd->subtype = AT_AddColumnToView; + atcmd->def = lfirst(c); + atcmds = lappend(atcmds, atcmd); + } + AlterTableInternal(viewOid, atcmds, true); + } + + /* * Create a tuple descriptor to compare against the existing view, and ! * verify that the old column list is an initial prefix of the new ! * column list. */ descriptor = BuildDescForRelation(attrList); checkViewTupleDesc(descriptor, rel->rd_att); *************** *** 219,231 **** { int i; ! if (newdesc->natts != olddesc->natts) ereport(ERROR, (errcode(ERRCODE_INVALID_TABLE_DEFINITION), ! errmsg("cannot change number of columns in view"))); /* we can ignore tdhasoid */ ! for (i = 0; i < newdesc->natts; i++) { Form_pg_attribute newattr = newdesc->attrs[i]; Form_pg_attribute oldattr = olddesc->attrs[i]; --- 244,256 ---- { int i; ! if (newdesc->natts < olddesc->natts) ereport(ERROR, (errcode(ERRCODE_INVALID_TABLE_DEFINITION), ! errmsg("cannot drop columns from view"))); /* we can ignore tdhasoid */ ! for (i = 0; i < olddesc->natts; i++) { Form_pg_attribute newattr = newdesc->attrs[i]; Form_pg_attribute oldattr = olddesc->attrs[i]; *************** *** 234,240 **** if (newattr->attisdropped != oldattr->attisdropped) ereport(ERROR, (errcode(ERRCODE_INVALID_TABLE_DEFINITION), ! errmsg("cannot change number of columns in view"))); if (strcmp(NameStr(newattr->attname), NameStr(oldattr->attname)) != 0) ereport(ERROR, --- 259,265 ---- if (newattr->attisdropped != oldattr->attisdropped) ereport(ERROR, (errcode(ERRCODE_INVALID_TABLE_DEFINITION), ! errmsg("cannot drop columns from view"))); if (strcmp(NameStr(newattr->attname), NameStr(oldattr->attname)) != 0) ereport(ERROR, Index: src/backend/parser/parse_utilcmd.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/backend/parser/parse_utilcmd.c,v retrieving revision 2.17 diff -c -r2.17 parse_utilcmd.c *** src/backend/parser/parse_utilcmd.c 1 Sep 2008 20:42:45 -0000 2.17 --- src/backend/parser/parse_utilcmd.c 2 Dec 2008 01:39:14 -0000 *************** *** 1721,1726 **** --- 1721,1727 ---- switch (cmd->subtype) { case AT_AddColumn: + case AT_AddColumnToView: { ColumnDef *def = (ColumnDef *) cmd->def; Index: src/include/nodes/parsenodes.h =================================================================== RCS file: /projects/cvsroot/pgsql/src/include/nodes/parsenodes.h,v retrieving revision 1.379 diff -c -r1.379 parsenodes.h *** src/include/nodes/parsenodes.h 24 Nov 2008 08:46:04 -0000 1.379 --- src/include/nodes/parsenodes.h 2 Dec 2008 01:39:17 -0000 *************** *** 989,994 **** --- 989,995 ---- typedef enum AlterTableType { AT_AddColumn, /* add column */ + AT_AddColumnToView, /* implicitly via CREATE OR REPLACE VIEW */ AT_ColumnDefault, /* alter column default */ AT_DropNotNull, /* alter column drop not null */ AT_SetNotNull, /* alter column set not null */ Index: src/test/regress/expected/create_view.out =================================================================== RCS file: /projects/cvsroot/pgsql/src/test/regress/expected/create_view.out,v retrieving revision 1.13 diff -c -r1.13 create_view.out *** src/test/regress/expected/create_view.out 11 Jun 2008 21:53:49 -0000 1.13 --- src/test/regress/expected/create_view.out 2 Dec 2008 01:39:18 -0000 *************** *** 49,63 **** -- should fail CREATE OR REPLACE VIEW viewtest AS SELECT a FROM viewtest_tbl WHERE a <> 20; ! ERROR: cannot change number of columns in view -- should fail CREATE OR REPLACE VIEW viewtest AS SELECT 1, * FROM viewtest_tbl; ! ERROR: cannot change number of columns in view -- should fail CREATE OR REPLACE VIEW viewtest AS SELECT a, b::numeric FROM viewtest_tbl; ERROR: cannot change data type of view column "b" DROP VIEW viewtest; DROP TABLE viewtest_tbl; -- tests for temporary views --- 49,66 ---- -- should fail CREATE OR REPLACE VIEW viewtest AS SELECT a FROM viewtest_tbl WHERE a <> 20; ! ERROR: cannot drop columns from view -- should fail CREATE OR REPLACE VIEW viewtest AS SELECT 1, * FROM viewtest_tbl; ! ERROR: column "b" of relation "viewtest" already exists -- should fail CREATE OR REPLACE VIEW viewtest AS SELECT a, b::numeric FROM viewtest_tbl; ERROR: cannot change data type of view column "b" + -- should work + CREATE OR REPLACE VIEW viewtest AS + SELECT a, b, 0 AS c FROM viewtest_tbl; DROP VIEW viewtest; DROP TABLE viewtest_tbl; -- tests for temporary views Index: src/test/regress/sql/create_view.sql =================================================================== RCS file: /projects/cvsroot/pgsql/src/test/regress/sql/create_view.sql,v retrieving revision 1.7 diff -c -r1.7 create_view.sql *** src/test/regress/sql/create_view.sql 7 Apr 2005 01:51:41 -0000 1.7 --- src/test/regress/sql/create_view.sql 2 Dec 2008 01:39:19 -0000 *************** *** 61,66 **** --- 61,70 ---- CREATE OR REPLACE VIEW viewtest AS SELECT a, b::numeric FROM viewtest_tbl; + -- should work + CREATE OR REPLACE VIEW viewtest AS + SELECT a, b, 0 AS c FROM viewtest_tbl; + DROP VIEW viewtest; DROP TABLE viewtest_tbl;
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers