Re: Proof of concept: auto updatable views [Review of Patch]

On 18 September 2012 14:23, Amit kapila <amit(dot)kapila(at)huawei(dot)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