Re: WIP: Automatic view update rules

--On Dienstag, November 11, 2008 23:06:08 -0500 Robert Haas
<robertmhaas(at)gmail(dot)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