From: | Bernd Helmle <mailings(at)oopsware(dot)de> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: WIP: Automatic view update rules |
Date: | 2008-11-13 17:15:19 |
Message-ID: | 7F84532C73CB3279DE8436E0@imhotep.credativ.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
--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
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2008-11-13 17:49:10 | Re: pg_filedump for CVS HEAD |
Previous Message | Alvaro Herrera | 2008-11-13 17:07:09 | pg_filedump for CVS HEAD |