> 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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
