Re: [HACKERS] CREATE OR REPLACE VIEW bug

[Tom Lane]

Dean Rasheed  writes:
> 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.

Looks reasonable in a quick once-over.  I didn't test it.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] CREATE OR REPLACE VIEW bug

[Dean Rasheed]

On 17 December 2016 at 15:42, Dean Rasheed  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 

[HACKERS] CREATE OR REPLACE VIEW bug

[Dean Rasheed]

It seems that there is a bug in CREATE OR REPLACE VIEW's handling of
WITH CHECK OPTION (noticed while thinking about the recent change to
pg_dump's handling of circular dependencies in views -- d8c05af). If
you use CREATE OR REPLACE VIEW on a view that isn't auto-updatable and
turn it into one that is, and at the same time attempt to add a WITH
CHECK OPTION (which is exactly what pg_dump will now do) it fails:

CREATE TABLE t1 (a int);
CREATE VIEW v1 AS SELECT null::int AS a;
CREATE OR REPLACE VIEW v1 AS SELECT * FROM t1 WHERE a > 0 WITH CHECK OPTION;

ERROR:  WITH CHECK OPTION is supported only on automatically updatable views
HINT:  Views that do not select from a single table or view are not
automatically updatable.


The problem is that before updating the view's query, DefineView()
calls DefineVirtualRelation() which attempts to add the new check
option to the existing view via the ALTER VIEW mechanism, and that
fails because the new check option isn't valid against the old view
query.

So if we're going to use the ALTER VIEW mechanism to update the view's
options, which is probably the most convenient way to do it,
DefineView()/DefineVirtualRelation() will need a little re-jigging to
do things in the required order.

I'll try to knock up a patch to do that.

Regards,
Dean


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers