On 18 September 2012 14:23, Amit kapila <amit.kap...@huawei.com> wrote: > Please find the review of the patch. >
Thanks for the review. Attached is an updated patch, and I've include some responses to specific review comments below. > Extra test cases that can be added to regression suite are as below: > > 1. where clause in view select statement I have modified my original test cases to include WHERE clauses in the view definitions and confirmed using EXPLAIN that they are picked up as expected by DML statements. > 2. ORDER BY, FOR, FETCH. The parser turns FETCH FIRST/NEXT into LIMIT before the query reaches the rewriter, so I don't think there is much point having separate tests for those cases. > 3. Temp views, views on temp tables. Yes that just works. > 4. Target entry JOIN, VALUES, FUNCTION I added a test with VALUES in the rangetable. The JOIN case is already covered by the existing test with multiple base relations, and the FUNCTION case is covered by the ro_view12 test. > 5. Toast column I see no reason why that would be a problem. It just works. > 6. System view Most system views aren't updatable because they involve multiple base relations, expressions in the target list or functions in the rangetable. This doesn't seem like a particularly useful use-case. > 7. Lateral and outer join This is covered by the existing test using multiple base relations. > 8. auto increment columns > 9. Triggers on tables > 10.View with default values I've added these and they appear to work as I would expect. > 11.Choosing base relation based on schema. > 12.SECURITY DEFINER function execution These also work, but I'm not sure that the tests are proving anything useful. > Code Review: > ------------------------ > > 1. In test_auto_update_view function > if (var->varattno == 0) > return "Views that refer to whole rows from the base > relation are not updatable"; > I have a doubt that when the above scenario will cover? And the examples > provided for whole row are working. > This protects against a whole row reference in the target list (for example CREATE VIEW test_view AS SELECT base_tbl FROM base_tbl). The case that is allowed is a whole row reference in the WHERE clause. > 2. In test_auto_update_view function > if (base_rte->rtekind != RTE_RELATION) > return "Views that are not based on tables or views are not > updatable"; > for view on sequences also the query is rewritten and giving error while > executing. > Is it possible to check for a particular relkind before rewriting query? > Updated, so now it raises the error in the rewriter rather than the executor. > 3. In function rewriteTargetView > if (tle->resjunk || tle->resno <= 0) > continue; > The above scenario is not possible as the junk is already removed in > above condition and also > the view which is refering to the system columns are not auto update > views. > OK, I've removed that check. The next test should catch anything unexpected that gets through. > 4. In function rewriteTargetView > if (view_tle == NULL) > elog(ERROR, "View column %d not found", tle->resno); > The parsetree targetlist is already validated with view targetlist during > transformstmt. > Giving an ERROR is fine here? Shouldn't it be Assert? > I think the elog(ERROR) is correct here, otherwise we'd be crashing. It ought to be impossible but it's not completely obvious that it can't somehow happen. > 5. if any derived columns are present on the view, at least UPDATE operation > can be allowed for columns other than derived columns. > Yes, but I think that's the subject for another patch. In this patch, I'm just aiming to implement the SQL-92 feature. > 6. name test_auto_update_view can be changed. The word test can be changed. > OK, I've renamed it to is_view_auto_updatable(). > 7. From function get_view_query(), error message : "invalid _RETURN rule > action specification" might not make much sense to user > who is inserting in a view. > This is an internal elog() error, rather than a user-facing error. It should not happen in practice, unless perhaps the user has been messing with their system catalogs. > Defects from test > --------------------------- > > 1. With a old database and new binaries the following test code results in > wrong way. > > CREATE TABLE base_tbl (a int PRIMARY KEY, b text DEFAULT 'Unspecified'); > INSERT INTO base_tbl VALUES (1, 'Row 1'); > INSERT INTO base_tbl VALUES (2, 'Row 2'); > > CREATE VIEW rw_view1 AS SELECT * FROM base_tbl; > > SELECT table_name, is_updatable, is_insertable_into > FROM information_schema.views where table_name = 'rw_view1'; > > This will show is_insertable_into as 'no'. However below SQL statement > is success > > INSERT INTO rw_view1 VALUES (3, 'Row 3'); > That's because the information_schema needs updating in your old database, which I think means that the catalog version number needs to be bumped when/if it is committed. > 2. User-1: > Table and views are created under user-1. > Grant select and insert permission to user-2 on table and view. > Alter the view owner as user-3. > > User-2: > Try to insert into the view is failing because of permission's even > though user-2 has rights on both table and view. > Yes that's correct behaviour, and is consistent with a SELECT from the view. The user that executes the query (user_2) needs permissions on the view, and the owner of the view/rule (user_3) needs permissions on the underlying table. It is not sufficient for user_2 to have permissions on the table because the permissions in the rewritten query are checked against the view/rule owner (user_3). See http://www.postgresql.org/docs/current/static/rules-privileges.html. > Documentation Improvements > -------------------------------------------- > 1. Under title Updateable Views - > > If the view satisifes all these conditions then it will be automatically > updatable. > This seems to be appearing both before and after the conditions of > non-updateable views. > 2. Grant of rights on views should be mentioned separatly. > I've tidied that up a bit and added a new paragraph with a reference to the section on rules and privileges. Regards, Dean
auto-update-views.patch.gz
Description: GNU Zip compressed data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
