Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 4da65a2

Browse files
committed
Code review for CREATE OR REPLACE VIEW patch. Do things in a saner order to
result in hopefully-less-confusing error messages when the new definition isn't compatible with the old; minor other cleanup.
1 parent 78b25fd commit 4da65a2

File tree

4 files changed

+48
-38
lines changed

4 files changed

+48
-38
lines changed

doc/src/sgml/ref/create_view.sgml

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<!--
2-
$PostgreSQL: pgsql/doc/src/sgml/ref/create_view.sgml,v 1.38 2008/12/06 23:22:46 momjian Exp $
2+
$PostgreSQL: pgsql/doc/src/sgml/ref/create_view.sgml,v 1.39 2008/12/15 21:35:31 tgl Exp $
33
PostgreSQL documentation
44
-->
55

@@ -38,9 +38,10 @@ CREATE [ OR REPLACE ] [ TEMP | TEMPORARY ] VIEW <replaceable class="PARAMETER">n
3838
<para>
3939
<command>CREATE OR REPLACE VIEW</command> is similar, but if a view
4040
of the same name already exists, it is replaced. The new query must
41-
generate all of the same columns that were generated by the original query
42-
in the same order and with the same data types, but may add additional
43-
columns to the end of the list.
41+
generate the same columns that were generated by the existing view query
42+
(that is, the same column names in the same order and with the same data
43+
types), but it may add additional columns to the end of the list. The
44+
calculations giving rise to the output columns may be completely different.
4445
</para>
4546

4647
<para>
@@ -77,7 +78,7 @@ CREATE [ OR REPLACE ] [ TEMP | TEMPORARY ] VIEW <replaceable class="PARAMETER">n
7778
</para>
7879
</listitem>
7980
</varlistentry>
80-
81+
8182
<varlistentry>
8283
<term><replaceable class="parameter">name</replaceable></term>
8384
<listitem>
@@ -164,7 +165,7 @@ CREATE VIEW comedies AS
164165
</programlisting>
165166
</para>
166167
</refsect1>
167-
168+
168169
<refsect1>
169170
<title>Compatibility</title>
170171

src/backend/commands/tablecmds.c

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.273 2008/12/13 19:13:44 tgl Exp $
11+
* $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.274 2008/12/15 21:35:31 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -3459,9 +3459,7 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
34593459
attrdesc;
34603460
HeapTuple reltup;
34613461
FormData_pg_attribute attribute;
3462-
int i;
3463-
int minattnum,
3464-
maxatts;
3462+
int newattnum;
34653463
char relkind;
34663464
HeapTuple typeTuple;
34673465
Oid typeOid;
@@ -3520,6 +3518,7 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
35203518
0, 0, 0);
35213519
if (!HeapTupleIsValid(reltup))
35223520
elog(ERROR, "cache lookup failed for relation %u", myrelid);
3521+
relkind = ((Form_pg_class) GETSTRUCT(reltup))->relkind;
35233522

35243523
/*
35253524
* this test is deliberately not attisdropped-aware, since if one tries to
@@ -3534,15 +3533,12 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
35343533
errmsg("column \"%s\" of relation \"%s\" already exists",
35353534
colDef->colname, RelationGetRelationName(rel))));
35363535

3537-
minattnum = ((Form_pg_class) GETSTRUCT(reltup))->relnatts;
3538-
relkind = ((Form_pg_class) GETSTRUCT(reltup))->relkind;
3539-
maxatts = minattnum + 1;
3540-
if (maxatts > MaxHeapAttributeNumber)
3536+
newattnum = ((Form_pg_class) GETSTRUCT(reltup))->relnatts + 1;
3537+
if (newattnum > MaxHeapAttributeNumber)
35413538
ereport(ERROR,
35423539
(errcode(ERRCODE_TOO_MANY_COLUMNS),
35433540
errmsg("tables can have at most %d columns",
35443541
MaxHeapAttributeNumber)));
3545-
i = minattnum + 1;
35463542

35473543
typeTuple = typenameType(NULL, colDef->typename, &typmod);
35483544
tform = (Form_pg_type) GETSTRUCT(typeTuple);
@@ -3551,14 +3547,15 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
35513547
/* make sure datatype is legal for a column */
35523548
CheckAttributeType(colDef->colname, typeOid);
35533549

3550+
/* construct new attribute's pg_attribute entry */
35543551
attribute.attrelid = myrelid;
35553552
namestrcpy(&(attribute.attname), colDef->colname);
35563553
attribute.atttypid = typeOid;
35573554
attribute.attstattarget = -1;
35583555
attribute.attlen = tform->typlen;
35593556
attribute.attcacheoff = -1;
35603557
attribute.atttypmod = typmod;
3561-
attribute.attnum = i;
3558+
attribute.attnum = newattnum;
35623559
attribute.attbyval = tform->typbyval;
35633560
attribute.attndims = list_length(colDef->typename->arrayBounds);
35643561
attribute.attstorage = tform->typstorage;
@@ -3578,7 +3575,7 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
35783575
/*
35793576
* Update number of attributes in pg_class tuple
35803577
*/
3581-
((Form_pg_class) GETSTRUCT(reltup))->relnatts = maxatts;
3578+
((Form_pg_class) GETSTRUCT(reltup))->relnatts = newattnum;
35823579

35833580
simple_heap_update(pgclass, &reltup->t_self, reltup);
35843581

@@ -3635,9 +3632,13 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
36353632
* returned by AddRelationNewConstraints, so that the right thing happens
36363633
* when a datatype's default applies.
36373634
*
3638-
* We skip this logic completely for views.
3635+
* We skip this step completely for views. For a view, we can only get
3636+
* here from CREATE OR REPLACE VIEW, which historically doesn't set up
3637+
* defaults, not even for domain-typed columns. And in any case we mustn't
3638+
* invoke Phase 3 on a view, since it has no storage.
36393639
*/
3640-
if (relkind != RELKIND_VIEW) {
3640+
if (relkind != RELKIND_VIEW)
3641+
{
36413642
defval = (Expr *) build_column_default(rel, attribute.attnum);
36423643

36433644
if (!defval && GetDomainConstraints(typeOid) != NIL)
@@ -3680,7 +3681,7 @@ ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
36803681
/*
36813682
* Add needed dependency entries for the new column.
36823683
*/
3683-
add_column_datatype_dependency(myrelid, i, attribute.atttypid);
3684+
add_column_datatype_dependency(myrelid, newattnum, attribute.atttypid);
36843685
}
36853686

36863687
/*

src/backend/commands/view.c

Lines changed: 25 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/commands/view.c,v 1.108 2008/12/06 23:22:46 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/commands/view.c,v 1.109 2008/12/15 21:35:31 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -165,6 +165,9 @@ DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
165165
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
166166
RelationGetRelationName(rel));
167167

168+
/* Also check it's not in use already */
169+
CheckTableNotInUse(rel, "CREATE OR REPLACE VIEW");
170+
168171
/*
169172
* Due to the namespace visibility rules for temporary objects, we
170173
* should only end up replacing a temporary view with another
@@ -173,37 +176,41 @@ DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
173176
Assert(relation->istemp == rel->rd_istemp);
174177

175178
/*
176-
* If new attributes have been added, we must modify the pre-existing
177-
* view.
178-
*/
179-
if (list_length(attrList) > rel->rd_att->natts) {
179+
* Create a tuple descriptor to compare against the existing view, and
180+
* verify that the old column list is an initial prefix of the new
181+
* column list.
182+
*/
183+
descriptor = BuildDescForRelation(attrList);
184+
checkViewTupleDesc(descriptor, rel->rd_att);
185+
186+
/*
187+
* If new attributes have been added, we must add pg_attribute entries
188+
* for them. It is convenient (although overkill) to use the ALTER
189+
* TABLE ADD COLUMN infrastructure for this.
190+
*/
191+
if (list_length(attrList) > rel->rd_att->natts)
192+
{
180193
List *atcmds = NIL;
181194
ListCell *c;
182195
int skip = rel->rd_att->natts;
183196

184-
foreach(c, attrList) {
197+
foreach(c, attrList)
198+
{
185199
AlterTableCmd *atcmd;
186200

187-
if (skip > 0) {
188-
--skip;
201+
if (skip > 0)
202+
{
203+
skip--;
189204
continue;
190205
}
191206
atcmd = makeNode(AlterTableCmd);
192207
atcmd->subtype = AT_AddColumnToView;
193-
atcmd->def = lfirst(c);
208+
atcmd->def = (Node *) lfirst(c);
194209
atcmds = lappend(atcmds, atcmd);
195210
}
196211
AlterTableInternal(viewOid, atcmds, true);
197212
}
198213

199-
/*
200-
* Create a tuple descriptor to compare against the existing view, and
201-
* verify that the old column list is an initial prefix of the new
202-
* column list.
203-
*/
204-
descriptor = BuildDescForRelation(attrList);
205-
checkViewTupleDesc(descriptor, rel->rd_att);
206-
207214
/*
208215
* Seems okay, so return the OID of the pre-existing view.
209216
*/
@@ -238,6 +245,7 @@ DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
238245
* Verify that tupledesc associated with proposed new view definition
239246
* matches tupledesc of old view. This is basically a cut-down version
240247
* of equalTupleDescs(), with code added to generate specific complaints.
248+
* Also, we allow the new tupledesc to have more columns than the old.
241249
*/
242250
static void
243251
checkViewTupleDesc(TupleDesc newdesc, TupleDesc olddesc)

src/test/regress/expected/create_view.out

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ ERROR: cannot drop columns from view
5353
-- should fail
5454
CREATE OR REPLACE VIEW viewtest AS
5555
SELECT 1, * FROM viewtest_tbl;
56-
ERROR: column "b" of relation "viewtest" already exists
56+
ERROR: cannot change name of view column "a"
5757
-- should fail
5858
CREATE OR REPLACE VIEW viewtest AS
5959
SELECT a, b::numeric FROM viewtest_tbl;

0 commit comments

Comments
 (0)