From: Dean Rasheed Date: Wed, 21 Dec 2016 17:01:52 +0000 (+0000) Subject: Fix order of operations in CREATE OR REPLACE VIEW. X-Git-Tag: REL9_6_2~72 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=a46ee6b30ae9cba3ece5b4b5912969c74969f274;p=postgresql Fix order of operations in CREATE OR REPLACE VIEW. When CREATE OR REPLACE VIEW acts on an existing view, don't update the view options until after the view query has been updated. This is necessary in the case where CREATE OR REPLACE VIEW is used on an existing view that is not updatable, and the new view is updatable and specifies the WITH CHECK OPTION. In this case, attempting to apply the new options to the view before updating its query fails, because the options are applied using the ALTER TABLE infrastructure which checks that WITH CHECK OPTION is only applied to an updatable view. If new columns are being added to the view, that is also done using the ALTER TABLE infrastructure, but it is important that that still be done before updating the view query, because the rules system checks that the query columns match those on the view relation. Added a comment to explain that, in case someone is tempted to move that to where the view options are now being set. Back-patch to 9.4 where WITH CHECK OPTION was added. Report: https://postgr.es/m/CAEZATCUp%3Dz%3Ds4SzZjr14bfct_bdJNwMPi-gFi3Xc5k1ntbsAgQ%40mail.gmail.com --- diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c index 085bf32320..d582ecd792 100644 --- 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; @@ -161,19 +159,14 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace, descriptor = BuildDescForRelation(attrList); 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, List *tlist, bool replace, 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, List *tlist, bool replace, 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,12 +246,19 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace, 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); Assert(address.objectId != InvalidOid); + + /* Make the new view relation visible */ + CommandCounterIncrement(); + + /* Store the query for the view */ + StoreViewQuery(address.objectId, viewParse, replace); + return address; } } @@ -529,16 +558,7 @@ DefineView(ViewStmt *stmt, const char *queryString) * 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 index f60991eed0..6162d40e59 100644 --- 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 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, 'ok'); -- ok +INSERT INTO v1 VALUES (-1, 'invalid'); -- should fail +ERROR: new row violates check option for view "v1" +DETAIL: Failing row contains (-1, 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 index 03c3f9d35e..868b22ae56 100644 --- 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 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, 'ok'); -- ok +INSERT INTO v1 VALUES (-1, 'invalid'); -- should fail + +DROP VIEW v1; +DROP TABLE t1;