Re: WIP: Automatic view update rules

--On Dienstag, November 11, 2008 23:06:08 -0500 Robert Haas 
<robertmhaas@gmail.com> wrote:

Thanks for your look at this. Unfortunately i was travelling the last 2 
days, so i didn't have time to reply earlier, sorry for that.

> I haven't done a full review of this patch, but here are some thoughts
> based on a quick read-through:
>
> - "make check" fails 16 of 118 tests for me with this patch applied.
>

Most of them are caused by additional NOTICE messages or unexpected 
additional rules in the rewriter regression tests. I don't see anything 
critical here.


> - There are some unnecessary hunks in this diff.  For example, some of
> the gram.y changes appear to move curly braces around, adjust spacing,

hmm i didn't see any changes to brackets or adjusting spaces...

> and replace true and false with TRUE and FALSE (I'm not 100% sure that
> the last of these isn't substantive... but I hope it's not).   The
> changes to rewriteDefine.c contain some commented-out declarations
> that need to be cleaned up, and at least one place where a blank line
> has been added.
>
> - The doc changes for ev_kind describe 'e' as meaning "rule was
> created by user", which confused me because why would you pick "e" for
> that?  Then I realized that "e" was probably intended to mean
> "explicit"; it would probably be good to work that word into the
> documentation of that value somehow.
>

okay

> - Should this be an optional behavior?  What if I don't WANT my view
> to be updateable?
>

I didn't see anything like this in the SQL92 draft...i thought if a view is 
updatable, it is, without any further adjustments. If you don't want your 
view updatable you have to REVOKE or GRANT your specific ACLs.

> - I am wondering if the old_tup_is_implicit variable started off its
> life as a boolean and morphed into a char.  It seems misnamed, now, at
> any rate.
>

agreed

> - The capitalization of deleteImplicitRulesOnEvent is inconsistent
> with the functions that immediately precede it in rewriteRemove.h.  I
> think the "d" should be capitalized.  checkTree() also uses this style
> of capitalization, which I haven't seen elsewhere in the source tree.
>

agreed

> - This:
>
> rhaas=# create table baz (a integer, b integer);
> CREATE TABLE
> rhaas=# create or replace view bar as select * from baz;
> NOTICE:  CREATE VIEW will create implicit INSERT/UPDATE/DELETE rules
> CREATE VIEW
>
> Generates this update rule:
>
> ON UPDATE TO bar DO INSTEAD  UPDATE ONLY foo SET a = new.a, b = new.b
>   WHERE
>         CASE
>             WHEN old.a IS NOT NULL THEN old.a = foo.a
>             ELSE foo.a IS NULL
>         END AND
>         CASE
>             WHEN old.b IS NOT NULL THEN old.b = foo.b
>             ELSE foo.b IS NULL
>         END
>   RETURNING new.a, new.b
>
> It seems like this could be simplified using IS NOT DISTINCT FROM.
>


I'll look at this.



--  Thanks
                   Bernd