On 17 December 2016 at 15:42, Dean Rasheed <dean.a.rash...@gmail.com> wrote:
> It seems that there is a bug in CREATE OR REPLACE VIEW...
>
> DefineView()/DefineVirtualRelation() will need a little re-jigging to
> do things in the required order.
...and the required order for existing views is
1. Add any new columns
2. Add rules to store the new query
3. Update the view options
because 2 will fail if the view's columns don't match the query's columns.
Attached is a patch enforcing this order and adding some comments to
make it clear why the order matters here.
Barring objections I'll back-patch this to 9.4 where WCO was added.
Regards,
Dean
diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c
new file mode 100644
index c6b0e4f..414507f
--- a/src/backend/commands/view.c
+++ b/src/backend/commands/view.c
@@ -59,15 +59,13 @@ validateWithCheckOption(char *value)
/*---------------------------------------------------------------------
* DefineVirtualRelation
*
- * Create the "view" relation. `DefineRelation' does all the work,
- * we just provide the correct arguments ... at least when we're
- * creating a view. If we're updating an existing view, we have to
- * work harder.
+ * Create a view relation and use the rules system to store the query
+ * for the view.
*---------------------------------------------------------------------
*/
static ObjectAddress
DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace,
- List *options)
+ List *options, Query *viewParse)
{
Oid viewOid;
LOCKMODE lockmode;
@@ -162,18 +160,13 @@ DefineVirtualRelation(RangeVar *relation
checkViewTupleDesc(descriptor, rel->rd_att);
/*
- * The new options list replaces the existing options list, even if
- * it's empty.
- */
- atcmd = makeNode(AlterTableCmd);
- atcmd->subtype = AT_ReplaceRelOptions;
- atcmd->def = (Node *) options;
- atcmds = lappend(atcmds, atcmd);
-
- /*
* 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.
+ *
+ * Note that we must do this before updating the query for the view,
+ * since the rules system requires that the correct view columns be in
+ * place when defining the new rules.
*/
if (list_length(attrList) > rel->rd_att->natts)
{
@@ -192,9 +185,38 @@ DefineVirtualRelation(RangeVar *relation
atcmd->def = (Node *) lfirst(c);
atcmds = lappend(atcmds, atcmd);
}
+
+ AlterTableInternal(viewOid, atcmds, true);
+
+ /* Make the new view columns visible */
+ CommandCounterIncrement();
}
- /* OK, let's do it. */
+ /*
+ * Update the query for the view.
+ *
+ * Note that we must do this before updating the view options, because
+ * the new options may not be compatible with the old view query (for
+ * example if we attempt to add the WITH CHECK OPTION, we require that
+ * the new view be automatically updatable, but the old view may not
+ * have been).
+ */
+ StoreViewQuery(viewOid, viewParse, replace);
+
+ /* Make the new view query visible */
+ CommandCounterIncrement();
+
+ /*
+ * Finally update the view options.
+ *
+ * The new options list replaces the existing options list, even if
+ * it's empty.
+ */
+ atcmd = makeNode(AlterTableCmd);
+ atcmd->subtype = AT_ReplaceRelOptions;
+ atcmd->def = (Node *) options;
+ atcmds = list_make1(atcmd);
+
AlterTableInternal(viewOid, atcmds, true);
ObjectAddressSet(address, RelationRelationId, viewOid);
@@ -211,7 +233,7 @@ DefineVirtualRelation(RangeVar *relation
ObjectAddress address;
/*
- * now set the parameters for keys/inheritance etc. All of these are
+ * Set the parameters for keys/inheritance etc. All of these are
* uninteresting for views...
*/
createStmt->relation = relation;
@@ -224,13 +246,20 @@ DefineVirtualRelation(RangeVar *relation
createStmt->if_not_exists = false;
/*
- * finally create the relation (this will error out if there's an
- * existing view, so we don't need more code to complain if "replace"
- * is false).
+ * Create the relation (this will error out if there's an existing
+ * view, so we don't need more code to complain if "replace" is
+ * false).
*/
address = DefineRelation(createStmt, RELKIND_VIEW, InvalidOid, NULL,
NULL);
Assert(address.objectId != InvalidOid);
+
+ /* Make the new view relation visible */
+ CommandCounterIncrement();
+
+ /* Store the query for the view */
+ StoreViewQuery(address.objectId, viewParse, replace);
+
return address;
}
}
@@ -530,16 +559,7 @@ DefineView(ViewStmt *stmt, const char *q
* aborted.
*/
address = DefineVirtualRelation(view, viewParse->targetList,
- stmt->replace, stmt->options);
-
- /*
- * The relation we have just created is not visible to any other commands
- * running with the same transaction & command id. So, increment the
- * command id counter (but do NOT pfree any memory!!!!)
- */
- CommandCounterIncrement();
-
- StoreViewQuery(address.objectId, viewParse, stmt->replace);
+ stmt->replace, stmt->options, viewParse);
return address;
}
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
new file mode 100644
index 79ddbde..f25b68f
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -2459,3 +2459,16 @@ DROP VIEW v2;
DROP VIEW v1;
DROP TABLE t2;
DROP TABLE t1;
+--
+-- Test CREATE OR REPLACE VIEW turning a non-updatable view into an
+-- auto-updatable view and adding check options in a single step
+--
+CREATE TABLE t1 (a int, b float, c text);
+CREATE VIEW v1 AS SELECT null::int AS a;
+CREATE OR REPLACE VIEW v1 AS SELECT * FROM t1 WHERE a > 0 WITH CHECK OPTION;
+INSERT INTO v1 VALUES (1, 2.0, 'ok'); -- ok
+INSERT INTO v1 VALUES (-1, -2.0, 'invalid'); -- should fail
+ERROR: new row violates check option for view "v1"
+DETAIL: Failing row contains (-1, -2, invalid).
+DROP VIEW v1;
+DROP TABLE t1;
diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql
new file mode 100644
index 03c3f9d..f52f5c3
--- a/src/test/regress/sql/updatable_views.sql
+++ b/src/test/regress/sql/updatable_views.sql
@@ -1098,3 +1098,17 @@ DROP VIEW v2;
DROP VIEW v1;
DROP TABLE t2;
DROP TABLE t1;
+
+--
+-- Test CREATE OR REPLACE VIEW turning a non-updatable view into an
+-- auto-updatable view and adding check options in a single step
+--
+CREATE TABLE t1 (a int, b float, c text);
+CREATE VIEW v1 AS SELECT null::int AS a;
+CREATE OR REPLACE VIEW v1 AS SELECT * FROM t1 WHERE a > 0 WITH CHECK OPTION;
+
+INSERT INTO v1 VALUES (1, 2.0, 'ok'); -- ok
+INSERT INTO v1 VALUES (-1, -2.0, 'invalid'); -- should fail
+
+DROP VIEW v1;
+DROP TABLE t1;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
