]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Code review for CREATE OR REPLACE VIEW patch. Do things in a saner order to
authorTom Lane <tgl@sss.pgh.pa.us>
Mon, 15 Dec 2008 21:35:31 +0000 (21:35 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Mon, 15 Dec 2008 21:35:31 +0000 (21:35 +0000)
result in hopefully-less-confusing error messages when the new definition
isn't compatible with the old; minor other cleanup.

doc/src/sgml/ref/create_view.sgml
src/backend/commands/tablecmds.c
src/backend/commands/view.c
src/test/regress/expected/create_view.out

index 7cbbdbe1ced77d66d9b855ca3e74d211c34c7b79..e0347a1919d324676aa58412308e8ab0df2fe94a 100644 (file)
@@ -1,5 +1,5 @@
 <!--
-$PostgreSQL: pgsql/doc/src/sgml/ref/create_view.sgml,v 1.38 2008/12/06 23:22:46 momjian Exp $
+$PostgreSQL: pgsql/doc/src/sgml/ref/create_view.sgml,v 1.39 2008/12/15 21:35:31 tgl Exp $
 PostgreSQL documentation
 -->
 
@@ -38,9 +38,10 @@ CREATE [ OR REPLACE ] [ TEMP | TEMPORARY ] VIEW <replaceable class="PARAMETER">n
   <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.
+   generate the same columns that were generated by the existing view query
+   (that is, the same column names in the same order and with the same data
+   types), but it may add additional columns to the end of the list.  The
+   calculations giving rise to the output columns may be completely different.
   </para>
 
   <para>
@@ -77,7 +78,7 @@ CREATE [ OR REPLACE ] [ TEMP | TEMPORARY ] VIEW <replaceable class="PARAMETER">n
      </para>
     </listitem>
    </varlistentry>
-   
+
    <varlistentry>
     <term><replaceable class="parameter">name</replaceable></term>
     <listitem>
@@ -164,7 +165,7 @@ CREATE VIEW comedies AS
 </programlisting>
   </para>
  </refsect1>
+
  <refsect1>
   <title>Compatibility</title>
 
index 6c60ddd5c104f79909116f844a56df99bc7747bb..9f34c735028d54b0ef38ddeba301b7cb8768d6ef 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.273 2008/12/13 19:13:44 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.274 2008/12/15 21:35:31 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -3459,9 +3459,7 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
                                attrdesc;
        HeapTuple       reltup;
        FormData_pg_attribute attribute;
-       int                     i;
-       int                     minattnum,
-                               maxatts;
+       int                     newattnum;
        char            relkind;
        HeapTuple       typeTuple;
        Oid                     typeOid;
@@ -3520,6 +3518,7 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
                                                                0, 0, 0);
        if (!HeapTupleIsValid(reltup))
                elog(ERROR, "cache lookup failed for relation %u", myrelid);
+       relkind = ((Form_pg_class) GETSTRUCT(reltup))->relkind;
 
        /*
         * this test is deliberately not attisdropped-aware, since if one tries to
@@ -3534,15 +3533,12 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
                                 errmsg("column \"%s\" of relation \"%s\" already exists",
                                                colDef->colname, RelationGetRelationName(rel))));
 
-       minattnum = ((Form_pg_class) GETSTRUCT(reltup))->relnatts;
-       relkind = ((Form_pg_class) GETSTRUCT(reltup))->relkind;
-       maxatts = minattnum + 1;
-       if (maxatts > MaxHeapAttributeNumber)
+       newattnum = ((Form_pg_class) GETSTRUCT(reltup))->relnatts + 1;
+       if (newattnum > MaxHeapAttributeNumber)
                ereport(ERROR,
                                (errcode(ERRCODE_TOO_MANY_COLUMNS),
                                 errmsg("tables can have at most %d columns",
                                                MaxHeapAttributeNumber)));
-       i = minattnum + 1;
 
        typeTuple = typenameType(NULL, colDef->typename, &typmod);
        tform = (Form_pg_type) GETSTRUCT(typeTuple);
@@ -3551,6 +3547,7 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
        /* make sure datatype is legal for a column */
        CheckAttributeType(colDef->colname, typeOid);
 
+       /* construct new attribute's pg_attribute entry */
        attribute.attrelid = myrelid;
        namestrcpy(&(attribute.attname), colDef->colname);
        attribute.atttypid = typeOid;
@@ -3558,7 +3555,7 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
        attribute.attlen = tform->typlen;
        attribute.attcacheoff = -1;
        attribute.atttypmod = typmod;
-       attribute.attnum = i;
+       attribute.attnum = newattnum;
        attribute.attbyval = tform->typbyval;
        attribute.attndims = list_length(colDef->typename->arrayBounds);
        attribute.attstorage = tform->typstorage;
@@ -3578,7 +3575,7 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
        /*
         * Update number of attributes in pg_class tuple
         */
-       ((Form_pg_class) GETSTRUCT(reltup))->relnatts = maxatts;
+       ((Form_pg_class) GETSTRUCT(reltup))->relnatts = newattnum;
 
        simple_heap_update(pgclass, &reltup->t_self, reltup);
 
@@ -3635,9 +3632,13 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
         * returned by AddRelationNewConstraints, so that the right thing happens
         * when a datatype's default applies.
         *
-        * We skip this logic completely for views.
+        * We skip this step completely for views.  For a view, we can only get
+        * here from CREATE OR REPLACE VIEW, which historically doesn't set up
+        * defaults, not even for domain-typed columns.  And in any case we mustn't
+        * invoke Phase 3 on a view, since it has no storage.
         */
-       if (relkind != RELKIND_VIEW) {
+       if (relkind != RELKIND_VIEW)
+       {
                defval = (Expr *) build_column_default(rel, attribute.attnum);
 
                if (!defval && GetDomainConstraints(typeOid) != NIL)
@@ -3680,7 +3681,7 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
        /*
         * Add needed dependency entries for the new column.
         */
-       add_column_datatype_dependency(myrelid, i, attribute.atttypid);
+       add_column_datatype_dependency(myrelid, newattnum, attribute.atttypid);
 }
 
 /*
index 23a03d28a9ccc89215d1169353473f4ca9a5f058..3b589d49e813bfed683dc51a48558df78d4e988b 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/commands/view.c,v 1.108 2008/12/06 23:22:46 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/commands/view.c,v 1.109 2008/12/15 21:35:31 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -165,6 +165,9 @@ DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
                        aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
                                                   RelationGetRelationName(rel));
 
+               /* Also check it's not in use already */
+               CheckTableNotInUse(rel, "CREATE OR REPLACE VIEW");
+
                /*
                 * Due to the namespace visibility rules for temporary objects, we
                 * should only end up replacing a temporary view with another
@@ -173,37 +176,41 @@ DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
                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) {
+                * 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);
+
+               /*
+                * If new attributes have been added, we must add pg_attribute entries
+                * for them.  It is convenient (although overkill) to use the ALTER
+                * TABLE ADD COLUMN infrastructure for this.
+                */
+               if (list_length(attrList) > rel->rd_att->natts)
+               {
                        List            *atcmds = NIL;
                        ListCell        *c;
                        int                     skip = rel->rd_att->natts;
 
-                       foreach(c, attrList) {
+                       foreach(c, attrList)
+                       {
                                AlterTableCmd *atcmd;
 
-                               if (skip > 0) {
-                                       --skip;
+                               if (skip > 0)
+                               {
+                                       skip--;
                                        continue;
                                }
                                atcmd = makeNode(AlterTableCmd);
                                atcmd->subtype = AT_AddColumnToView;
-                               atcmd->def = lfirst(c);
+                               atcmd->def = (Node *) 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);
-
                /*
                 * Seems okay, so return the OID of the pre-existing view.
                 */
@@ -238,6 +245,7 @@ DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
  * Verify that tupledesc associated with proposed new view definition
  * matches tupledesc of old view.  This is basically a cut-down version
  * of equalTupleDescs(), with code added to generate specific complaints.
+ * Also, we allow the new tupledesc to have more columns than the old.
  */
 static void
 checkViewTupleDesc(TupleDesc newdesc, TupleDesc olddesc)
index e9b6c83e25d883affc095a332b2a83f5ce231029..90e6c70e572f716e419b3dff0b410d94aa84fd15 100644 (file)
@@ -53,7 +53,7 @@ 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
+ERROR:  cannot change name of view column "a"
 -- should fail
 CREATE OR REPLACE VIEW viewtest AS
        SELECT a, b::numeric FROM viewtest_tbl;