Re: Proof of concept: auto updatable views [Review of Patch] - Mailing list pgsql-hackers

On 31 August 2012 07:59, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 30 August 2012 20:05, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Sun, Aug 12, 2012 at 5:14 PM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:

> Here's an updated patch for the base feature (without support for
> security barrier views) with updated docs and regression tests.

Please find the review of the patch.

Basic stuff:
----------------------
- Patch applies OK
- Compiles cleanly with no warnings
- Regression tests pass.
- Documentation changes are mostly fine.

What it does:
------------------------

The non-select DML operations on views which are simple updatable (The condition checks can be found in test section), can rewrite the query and execute it even if the view don't have rules and instead of triggers.


Testing:
----------------
The following test is carried out on the patch.

1. create a view which can be automatically updated.
    - no of view columns are not matching with actual base relation
    - column order should be different with base relation order
    - view column aliases as another column name
    - view with a where condition
    - column contains some constraints.    
    - ORDER BY
    - FOR

2. create a view with all the features where automatically update is not possible.
    - Distinct clauses
    - Every TLE is not a column reference.
    - TLE appears more than once.
    - Refers to more than one base relation.
    - Other than relation in FROM clause.
    - GROUP BY or HAVING clauses.
    - set operations (UNION, INTERSECT or EXCEPT).
    - sub-queries in the WHERE clause.
    - DISTINCT ON clauses.
    - window functions.
    - CTEs (WITH or WITH RECURSIVE).
    - OFFSET or LIMIT clauses.
    - system columns.
    - security_barrier;
    - refers to whole rows of a table.
    - column permissions are not there for users.
    - refers to a sequence instead of relation.
    -

3. create a view which is having instead of triggers.
4. create a view which is having rules.
5. create a view on a base relation which is also a view of automatically updated.
6. create a view on a base relation which is also a view having instead of triggers.
7. create a view on a base relation which is also a view having rules.

Extra test cases that can be added to regression suite are as below:

1. where clause in view select statement
2. ORDER BY, FOR, FETCH.
3. Temp views, views on temp tables.
4. Target entry JOIN, VALUES, FUNCTION
5. Toast column
6. System view
7. Lateral and outer join
8. auto increment columns
9. Triggers on tables
10.View with default values
11.Choosing base relation based on schema.
12.SECURITY DEFINER function execution 

 The updated regression test sql file with extra test is attached with this mail, please check and add it to the patch.

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.

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?
   
   
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.
   
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?

5. if any derived columns are present on the view, at least UPDATE operation can be allowed for columns other than derived columns.  

6. name test_auto_update_view can be changed. The word test can be changed.

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.


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');              

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.

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.

With Regards,

Amit Kapila.